`asgiref` backward compatibility #1305

tony · 2022-01-03T18:15:24Z

Checklist

The bug is reproducible against the latest release or master.
There are no similar issues or pull requests to fix it yet.

Describe the bug

Hi, thank you for the project, this is a shot in the dark since it's not a channels bug!

#1100 pinned the asgiref dependency to >=3.4.0 for mypy purposes. It may be worth loosening it if it's only mypy as the restriction requires the user to use asgiref 3.4.0, which triggers a well-known but unsolved channels (a different package)

Alternatives to pulling in asgiref for typings:

Dropping the asgiref dependency and copying asgiref.typings to _types

_types can have compat-like functionality depending on the asgiref version installed so things so uvicorn can be independent of django and channels.
Loosening the constraint

Would mypy work if try/catch with ImportError was used? Would be willing to consider it as a compatibility workaround until ASGIRef 3.4.1 + Channels 3.0.3 causes non-deterministic 500 errors serving static files django/channels#1722 is resolved?
```
try:
    from asgiref.typing import WebSocketScope
except ImportError:
   from asgiref.typing import WebsocketScope
   WebSocketScope = WebsocketScope
```

A word of note:

This is for posterity, of course it's never needed for an independent project such as uvicorn to change it's pinning due to another package's issues, but noteworthy since channels is often used alongside uvicorn by users.

The reasons why I post this here is channel's bug at django/channels#1722 is prominent, its easiest apparent workaround is to decrease asgiref's version below <3.4.0, and the only reason I can see 3.4.0 was pinned in #1100 is mypy and there may be a way to satisfy mypy and relax the constraint.

I think it's up to the maintainer and I wanted to document the issue.

Steps to reproduce the bug

I believe this happens on sites where static files are served via the asgi server. This happens when there's a heavy load (such as selenium tests).

Expected behavior

Static files via dev servers should serve without raising deadlock errors

Actual behavior

No response

Debugging material

Assuming uvicode 0.16.0, it requires we use asgiref 3.4.0+

Error: 1-03 17:27:52 +0000] [9] [ERROR] Exception in ASGI application
Traceback (most recent call last):
  File "/opt/venv/lib/python3.10/site-packages/uvicorn/protocols/http/httptools_impl.py", line 376, in run_asgi
    result = await app(self.scope, self.receive, self.send)
  File "/opt/venv/lib/python3.10/site-packages/uvicorn/middleware/proxy_headers.py", line 75, in __call__
    return await self.app(scope, receive, send)
  File "/opt/venv/lib/python3.10/site-packages/uvicorn/middleware/asgi2.py", line 17, in __call__
    await instance(receive, send)
  File "/opt/venv/lib/python3.10/site-packages/channels/http.py", line 192, in __call__
    await self.handle(body_stream)
  File "/opt/venv/lib/python3.10/site-packages/asgiref/sync.py", line 409, in __call__
    raise RuntimeError(
RuntimeError: Single thread executor already being used, would deadlock

Environment

OS: Ubuntu latest via docker
uvicorn: 0.16.0
channels: 2.4.0
asgiref: 3.4.0 (breakage) 3.3.4 (works)
python 3.10

Additional context

No response

The text was updated successfully, but these errors were encountered:

tony · 2022-01-04T23:13:20Z

If it were me, I'd vendorize the typings again and check the asgiref.__version__ to handle at compatibility differences.

The reason why is asgiref 3.3 and 3.4, despite being minor version bumps and technically safer, are fundamentally different when placed into a server or CI environment. If you're using channels you've opened up a can of worms of static file bugs when channels and django comes into play.

Perhaps future asgiref releases won't be as drastic as 3.3 and 3.4, but I think that typings alone aren't enough of a reason to include the package in uvicorn and add a constraint. asgiref is not stable enough yet when other packages like channels, django and normal CI/devops is concerned.

While I see the ecosystem around it stabilizing over time, I am happy to open a PR to re-vendorize typings in and with multi-version asgiref compatibility (and tests).

Kludex · 2022-01-08T17:21:34Z

I'm willing to approve a PR with the vendor, and tests running against asgiref.typing. 👍

tony · 2022-01-08T17:22:26Z

@Kludex I will prepare one ASAP

Kludex · 2022-01-08T17:23:43Z

Cool. Thanks 👍

tony · 2022-01-08T18:34:22Z

Follow up in #1312

_types can have compat-like functionality depending on the asgiref version installed so things so uvicorn can be independent of django and channels.

That hypothesis isn't working out. I don't see a way to mypy or type annotations can branch based on asgiref version.

tomchristie · 2022-02-11T12:19:37Z

This is me buzzing by with the observation that just mellowing out and rolling with scope: dict is cool and relaxed and fuzzywuzzy, and makes for more friendly codebases. (At least from my perspective) 😌

tony · 2022-02-11T13:22:53Z

This is me buzzing by with the observation that just mellowing out and rolling with scope: dict is cool and relaxed and fuzzywuzzy, and makes for more friendly codebases. (At least from my perspective) 😌

opened at #1373

tony · 2022-02-11T14:19:35Z

@tomchristie Would you want to move this to discussion per #1374 until there's a consensus? Or fine keeping it as issue for now?

tomchristie · 2022-02-11T14:28:44Z

I'm not sure @tony - that's a good question.

I'd certainly suggest closing #1373 off, yes.

Since this issue is open, and since it's clear that it's behaviourally breaking something for you let's keep it open and get it resolved.

Your option (2.) seems like it might be a good approach to start with.
That'd be a nice low-footprint change, which I'm all in favour of.

tony · 2022-02-11T14:33:58Z

I will keep this issue open for now

I'd certainly suggest closing #1373 off, yes.

Can you clarify what's meant by closing it off? #1305 (comment) discussed using dict in favor of Scope.

tomchristie · 2022-02-11T14:47:02Z

Can you clarify what's meant by closing it off?

Yes. I think we should close #1373. (Without merging.)

#1305 (comment) discussed using dict in favor of Scope.

I think there's a discussion worth having there, but not we should definitely do this.

I will keep this issue open for now.

Agreed.

The lowest-possible change footprint PR is probably a good approach. If (2) would resolve the issue for you then I'd suggest we'd want to go with that.

We could later consider either vendoring asgiref as per #1312, or dropping it by relaxing the typing, but those options can start as a discussion. (And there's no rush on them if they're not behaviourally breaking things)

This commit will enable mypy strict mode, and update code accordingly. Type annotations are not used at runtime. The standard library `typing` module includes a `TYPE_CHECKING` constant that is `False` at runtime, but `True` when conducting static type checking prior to runtime. Type imports will be included under `if TYPE_CHECKING:` conditions. These conditions will be ignored when calculating test coverage. https://docs.python.org/3/library/typing.html The Python standard library `logging.config` module uses type stubs. The typeshed types for the `logging.config` module are used solely for type-checking usage of the `logging.config` module itself. They cannot be imported and used to type annotate other modules. For this reason, dict config types will be vendored into a module in the inboard package. https://github.com/python/typeshed/blob/main/stdlib/logging/config.pyi The ASGI application in `inboard.app.main_base` will be updated to ASGI3 and type-annotated with `asgiref.typing`. Note that, while Uvicorn uses `asgiref.typing`, Starlette does not. The type signature expected by the Starlette/FastAPI `TestClient` therefore does not match `asgiref.typing.ASGIApplication`. A mypy `type: ignore[arg-type]` comment will be used to resolve this difference. https://asgi.readthedocs.io/en/stable/specs/main.html Also note that, while the `asgiref` package was a runtime dependency of Uvicorn 0.17.6, it was later removed from Uvicorn's runtime dependencies in 0.18.0 (encode/uvicorn#1305, encode/uvicorn#1532). However, `asgiref` is still used to type-annotate Uvicorn, so any downstream projects like inboard that type-check Uvicorn objects must also install `asgiref`. Therefore, `asgiref` will be added to inboard's development dependencies to ensure that type checking continues to work as expected. A Uvicorn options type will be added to a new inboard types module. The Uvicorn options type will be a `TypedDict` with fields corresponding to arguments to `uvicorn.run`. This type can be used to check arguments passed to `uvicorn.run`, which is how `inboard.start` runs Uvicorn. Uvicorn 0.17.6 is not fully type-annotated, and Uvicorn does not ship with a `py.typed` marker file until 0.19.0. It would be convenient to generate types dynamically with something like `getattr(uvicorn.run, "__annotations__")` (Python 3.9 or earlier) or `inspect.get_annotations(uvicorn.run)` (Python 3.10 or later). https://docs.python.org/3/howto/annotations.html It could look something like this: ```py UvicornOptions = TypedDict( # type: ignore[misc] "UvicornOptions", inspect.get_annotations(uvicorn.run), total=False, ) ``` Note the `type: ignore[misc]` comment. Mypy raises a `misc` error: `TypedDict() expects a dictionary literal as the second argument`. Unfortunately, `TypedDict` types are not intended to be generated dynamically, because they exist for the benefit of static type checking (python/mypy#3932, python/mypy#4128, python/mypy#13940). Furthermore, prior to Uvicorn 0.18.0, `uvicorn.run()` didn't enumerate keyword arguments, but instead accepted `kwargs` and passed them to `uvicorn.Config.__init__()` (encode/uvicorn#1423). The annotations from `uvicorn.Config.__init__()` would need to be used instead. Even after Uvicorn 0.18.0, the signatures of the two functions are not exactly the same (encode/uvicorn#1545), so it helps to have a static type defined. There will be some other differences from `uvicorn.run()`: - The `app` argument to `uvicorn.run()` accepts an un-parametrized `Callable` because Uvicorn tests use callables (encode/uvicorn#1067). It is not necessary for other packages to accept `Callable`, and it would need to be parametrized to pass mypy strict mode anyway. For these reasons, `Callable` will not be accepted in this type. - The `log_config` argument will use the new inboard dict config type instead of `dict[str, Any]` for stricter type checking.

tony changed the title ~~Loosen asgiref dependency? (>=3.4.0 prevents a workaround of common channels bug)~~ asgiref backward compatibility Jan 5, 2022

Kludex added the enhancement label Jan 8, 2022

tony mentioned this issue Jan 8, 2022

Refactor/vendorize typings #1312

Closed

tony mentioned this issue Feb 11, 2022

chore(mypy): Loosen scope typing to dict #1373

Closed

br3ndonland mentioned this issue Apr 21, 2022

♻️ Use asgiref.typing instead of starlette.types encode/starlette#1374

Closed

16 tasks

This was referenced May 3, 2022

Remove asgiref from minimal requirements #1460

Closed

Replace asgiref for native types #1476

Closed

Kludex mentioned this issue Jun 12, 2022

Replace asgiref by asgi-types #1515

Closed

Kludex mentioned this issue Jun 21, 2022

Remove asgiref dependency #1532

Merged

euri10 closed this as completed in #1532 Jun 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`asgiref` backward compatibility #1305

`asgiref` backward compatibility #1305

tony commented Jan 3, 2022 •

edited

Loading

tony commented Jan 4, 2022 •

edited

Loading

Kludex commented Jan 8, 2022 •

edited

Loading

tony commented Jan 8, 2022

Kludex commented Jan 8, 2022

tony commented Jan 8, 2022 •

edited

Loading

tomchristie commented Feb 11, 2022

tony commented Feb 11, 2022

tony commented Feb 11, 2022

tomchristie commented Feb 11, 2022

tony commented Feb 11, 2022

tomchristie commented Feb 11, 2022

asgiref backward compatibility #1305

asgiref backward compatibility #1305

Comments

tony commented Jan 3, 2022 • edited Loading

Checklist

Describe the bug

Steps to reproduce the bug

Expected behavior

Actual behavior

Debugging material

Environment

Additional context

tony commented Jan 4, 2022 • edited Loading

Kludex commented Jan 8, 2022 • edited Loading

tony commented Jan 8, 2022

Kludex commented Jan 8, 2022

tony commented Jan 8, 2022 • edited Loading

tomchristie commented Feb 11, 2022

tony commented Feb 11, 2022

tony commented Feb 11, 2022

tomchristie commented Feb 11, 2022

tony commented Feb 11, 2022

tomchristie commented Feb 11, 2022

`asgiref` backward compatibility #1305

`asgiref` backward compatibility #1305

tony commented Jan 3, 2022 •

edited

Loading

tony commented Jan 4, 2022 •

edited

Loading

Kludex commented Jan 8, 2022 •

edited

Loading

tony commented Jan 8, 2022 •

edited

Loading