Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix middleware traceback fetching on Python 3.8+, fix ResourceWarnings in TestClient, fix CI build #1132

Merged
merged 3 commits into from
Jan 31, 2021

Conversation

JayH5
Copy link
Member

@JayH5 JayH5 commented Jan 30, 2021

This would ideally be 3 separate PRs, but all of these issues have broken the build on master so it'd be difficult to demonstrate them working separately. Still, let me know if you'd like me to split things up.

The issues are:

  1. Mypy 0.800 has some changes w.r.t module discovery. The mypy run was failing because mypy wanted the test functions in tests/middleware/* to have type annotations, despite this being disabled for tests submodules. Mypy was not correctly identifying that these tests were submodules due to the lack of an __init__.py file. See somebody having a similar problem in mypy.ini module-specific override doesn't work without __init__.py python/mypy#9974. An alternative (or something we could also do) would be to add namespace_packages = True to the config. Since we have __init__.py files everywhere else, I went with that option for now.
  2. Something in a dependency (pytest? Python itself?) changed so that we were having a bunch more ResourceWarnings detected due to unclosed sockets. These (Unix) sockets I believe were setup by the asyncio event loop as part of its internal machinery. In the TestClient, in the WebSocketTestSession we weren't closing the event loop used by the worker thread there. I changed the loop creation to be done inside the thread (since it is only used by the thread, and can never be used again once the thread has completed) and closed the loop at the end of its use (also in the thread). Possibly fixes fix Unraisable ResourceWarnings #1050
  3. In ServerErrorMiddleware we were using the undocumented exc_traceback attribute of TracebackError. This attribute was removed for bpo42482 in Python 3.9.1 & 3.8.7. I switched to using the __traceback__ attribute of exceptions which I think should have the same value (it at least produces the same HTML for the test case we have). I also added a check that the exception actually had a traceback, which we weren't doing previously but probably should've been. Fixes Test failure #1131, The debug mode does not work well #1126

@JayH5 JayH5 requested a review from a team January 30, 2021 22:21
Copy link
Member

@euri10 euri10 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something in a dependency (pytest? Python itself?)

It's pytest itself (since 6.2 https://docs.pytest.org/en/stable/usage.html#unraisable) and if you look it happens only on 3.8 and above as expected.
Since we turn warnings into exceptions then this popped off.
Uvicorn has a pending PR to do the same.

Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much ❤️

@florimondmanca florimondmanca changed the title Fix CI build (3 separate problems) Fix middleware traceback fetching on Python 3.8+, fix ResourceWarnings in TestClient, fix CI build Jan 31, 2021
@florimondmanca
Copy link
Member

@JayH5 I updated the title bc there are 2 items we'll want this PR to be included in the changelog for (fixing tracebacks, fixing ResourceWarning instances).

@florimondmanca
Copy link
Member

Depending on what's currently in master I guess we can follow up with a bug fix release?

@florimondmanca florimondmanca merged commit 62e95b8 into master Jan 31, 2021
@florimondmanca florimondmanca deleted the fix-build-2021 branch January 31, 2021 11:43
@JayH5
Copy link
Member Author

JayH5 commented Jan 31, 2021

Thanks @euri10 @florimondmanca ❤️

I'll look at doing a release soon.

@JayH5 JayH5 mentioned this pull request Feb 2, 2021
mweinelt added a commit to mweinelt/nixpkgs that referenced this pull request Mar 20, 2021
- ujson was dropped in encode/starlette#1047
- test_debug_html was fixed in encode/starlette#1132
risicle added a commit to risicle/nixpkgs that referenced this pull request Jun 1, 2021
newer python 3.8+ releases expose this breakage
encode/starlette#1131, fixed upstream in
encode/starlette#1132
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test failure fix Unraisable ResourceWarnings
3 participants