httpcore icon indicating copy to clipboard operation
httpcore copied to clipboard

Update types for h11 v0.13

Open zanieb opened this issue 2 years ago • 4 comments

Replaces #503, performing the additional work necessary for type checks to pass.

A cast is necessary because h11 does not expose the Sentinel type and uses it as a return type annotation instead of specifying the subtypes that can be returned.

event = cast(
    Union[h11.Event, h11.NEED_DATA, h11.PAUSED],
    self._h11_state.next_event(),
)

I've opened a patch to h11 to update next_event to have a return type that is narrower and a part of their public API: https://github.com/python-hyper/h11/pull/144

There is currently no handling in _receive_event for the h11.PAUSED sentinel event. I am not sure what behavior you'd like to add there and have left a stub that should be filled in before this is merged. The documentation for PAUSED can be found at https://h11.readthedocs.io/en/latest/api.html#flow-control

Note this partially addresses #509 by allowing a newer version of h11 to be used, but does not close the issue as it requests that the upper pin be removed entirely.

Closes #503 Closes #498

zanieb avatar Mar 25 '22 16:03 zanieb

I think it may not be a good idea to pin h11 under 0.14, it can cause unnecessary conflicts. See: https://hynek.me/articles/semver-will-not-save-you/

GalaxySnail avatar Apr 09 '22 08:04 GalaxySnail

I think it may not be a good idea to pin h11 under 0.14, it can cause unnecessary conflicts. See: https://hynek.me/articles/semver-will-not-save-you/

That sentiment is the point of issue #509. This pull request is to add compatibility which is necessary whether the pin is removed or not. There are other dependencies with upper versions in this project and that decision should be made separately from this fix.

zanieb avatar Apr 09 '22 18:04 zanieb

I think this is ready for review now @madkinsz

graingert avatar Jun 19 '22 17:06 graingert

I'm seeing a weird intermittent bug with https://lite.datasette.io/ which it looks like would be resolved by merging this PR!

  • https://github.com/simonw/datasette-lite/issues/42
  File "/lib/python3.10/asyncio/tasks.py", line 232, in __step
    result = coro.send(None)
  File "/lib/python3.10/site-packages/micropip/_micropip.py", line 310, in add_requirement
    return await self.add_requirement_inner(req)
  File "/lib/python3.10/site-packages/micropip/_micropip.py", line 391, in add_requirement_inner
    if self.check_version_satisfied(req):
  File "/lib/python3.10/site-packages/micropip/_micropip.py", line 337, in check_version_satisfied
    raise ValueError(
ValueError: Requested 'h11<0.13,>=0.11', but h11==0.13.0 is already installed

It looks like something deep in the complex world of Pyodide may be installing h11==0.13 - even though I've tried in my code to ensure that h11==0.12 is installed first, here: https://github.com/simonw/datasette-lite/blob/d64455c2b7d89a32b8537c63f46db3579f0de91f/webworker.js#L53-L54

I'm going to keep on looking for a workaround, but I thought I'd drop a note here about this issue in case anyone else runs into a similar problem and lands here from a search.

simonw avatar Aug 15 '22 23:08 simonw

@graingert I'm not sure the remaining optimizations regarding the overload are feasible in the short term. It may be worth merging the current improvements to unblock upgrades and address the overload related checks once we can implement that upstream.

Can we get another maintainer to take a look at the hot-loop cast concerns?

zanieb avatar Aug 26 '22 19:08 zanieb

Note https://github.com/python-hyper/h11/commit/04cc0f781c47ebfb9f9b188a8e8aa423f276c0b1 was merged a week ago so we can remove the aforementioned hot-loop cast once there's another h11 release.

zanieb avatar Sep 01 '22 16:09 zanieb

h11 just released v0.14.0. Does that mean this PR can remove the "hot loop cast" and move forward with another release for httpcore?

I have a server I deal with which behaves poorly and the h11 update will handle its behavior, so I look forward to having it make its way through h11 > httpcore > httpx releases.

eseglem avatar Sep 25 '22 17:09 eseglem

I'll look into updating this for h11 v0.14

Edit: See #579

zanieb avatar Sep 25 '22 18:09 zanieb

Thanks @madkinsz - should we close this off in favour of #579?

tomchristie avatar Sep 26 '22 11:09 tomchristie

I think that's the best approach!

zanieb avatar Sep 26 '22 14:09 zanieb