-
Notifications
You must be signed in to change notification settings - Fork 92
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
Unhandled KeyError with HTTP/2 #197
Comments
I just faced the same problem today. Apparently there is already a potential solution (Issue #50), and yes no troubles to end users just the above annoying error. |
As far as I can tell, #50 is a different error. I see this comment is the same error as I am getting, an unhandled IndexError on line 259 of protocol/h2.py. |
You are right. There are two messages errors there. Ours is a different one. It looks like is always present at ANY TLS HTTP/2 request even the simplest ones (e.g. several GETs to same endpoint). The call itself is honoured, but then sometimes there is an ASYNCIO "Task exception was never retrieved... future" thing. Maybe related to this stackoverflow question. I tried to debug, but I think we need an asyncio expert to explain what is happening (and when), for the async way of programming is new to me. The exception is raised during create_stream execution (def _create_stream in "hypercorn/protocol/h2.py"). Maybe a zoombie task or an exception not properly catched inside it... I don't know for sure. But, it is without a doubt related to a closed connection. |
Here is a boiled-down demonstration of what I have figured out: import asyncio
import traceback
class Stream:
def do_whatever(self):
pass
class StreamClosed:
stream_id: int
class StreamEnded:
stream_id: int
class H2Protocol:
def __init__(self):
self.streams = {}
async def stream_send(self, event):
if isinstance(event, StreamClosed):
await self._close_stream(event.stream_id)
elif isinstance(event, StreamEnded):
self.streams[event.stream_id].do_whatever()
async def _close_stream(self, stream_id):
self.streams.pop(stream_id)
async def run_task_group():
# Hypercorn runs a TaskGroup in hypercorn.asyncio.tcp_server
async with asyncio.TaskGroup() as task_group:
# There is a H2Protocol object that handles events for a dict of streams
h = H2Protocol()
# When a connection is made, a stream gets added to the dict
h.streams[3] = Stream()
# When the stream is closed, a StreamClosed event is sent to the protocol.
# This event causes the stream to be destroyed and forgotten.
event_1 = StreamClosed()
event_1.stream_id = 3
task_group.create_task(h.stream_send(event_1))
# Then some data is received that causes a StreamEnded event to be also sent.
# The event looks for the stream_id and raises a KeyError because the stream is not in the dict anymore.
event_2 = StreamEnded()
event_2.stream_id = 3
task_group.create_task(h.stream_send(event_2))
loop = asyncio.new_event_loop()
try:
loop.run_until_complete(run_task_group())
except ExceptionGroup as grp:
traceback.print_exc() This example prints a similar traceback to what I am getting. |
Great job. As far as I could see not all requests really create a stream object inside self.streams, so I decided to check if the ID is inside it (code below). However, it would be great to check if such a simple solution cover all cases. venv/lib/python3.10/site-packages/hypercorn/protocol/h2.py async def _handle_events(self, events: List[h2.events.Event]) -> None:
for event in events:
if isinstance(event, h2.events.RequestReceived):
if self.context.terminated.is_set():
...
elif isinstance(event, h2.events.StreamEnded):
+++ if event.stream_id in self.streams:
await self.streams[event.stream_id].handle(EndBody(stream_id=event.stream_id))
+++ else:
+++ print(f">>> WEIRD! {event.stream_id}") ## for debug purposes |
I found I can produce this error by closing Firefox and reopening it and revisiting my server. I suspect that part of the reason why I am getting this error is that my HTTP traffic goes through a proxy server which might not handle HTTP/2 perfectly. I tested running my exact same server on my local PC, and also visiting the site from an unproxied device, and I'm not getting the error. Still, the error should at least be caught since it turns up in real life in more than one situation. |
NAME="CentOS Linux" app, config = create_app()
def create_app():
app = FastAPI(lifespan=app_lifespan)
config = Config()
config.bind = ["0.0.0.0:6001"]
config.certfile = "cert.pem"
config.keyfile = "key.pem"
return app, config
def _exception_handler(lp, context):
exception = context.get("exception")
if isinstance(exception, ssl.SSLError):
pass
else:
lp.default_exception_handler(context)
if __name__ == '__main__':
loop = asyncio.new_event_loop()
asyncio.set_event_loop(loop)
loop.set_exception_handler(_exception_handler)
loop.run_until_complete(serve(app, config)) |
That seems to be a different error. Are you using Quart? @pgjones Any input on this issue? |
Hello! I finally was able to step through the code and here is what I have so far:
So, IMHO, basically the I hope I am on the correct track. I don't really know how best to resolve this issue. But I can submit a PR with a hacky proposal. However, I would love to fix it for the next release if someone could advise on the best approach. Thank you!!!! EDIT 1: I just read the entire thread and looks like most of the research was already done. Sorry! I didn't understand what was happening until I stepped through the code myself. Now I have duplicated the effort. However, I did try something similar to the solution proposed above.
This cause the streams dict to grow unbounded. So obviously a bad idea. Then I tried pretty much exactly the proposal above:
And I believe this seems to work, at least in my testing ... I would give this solution a 👍 !!!!!! EDIT 2: Just wanted to add that our app based on FastAPI and I can reproduce this error on every request locally on my laptop. This only happens on SSL. In my testing I am using chrome which is not using the clear text protocol but the other one (binary?). And I used a self-signed cert locally. Very easy to reproduce. |
Does 24aacf4 fix this? |
Looks like it should. Thank you! |
I am using hypercorn 0.16.0 and quart 0.19.4 on Debian 11 and getting this error periodically:
I don't need HTTP/2 for this site, so I would also be OK with just disabling it if someone can tell me how.
The error doesn't seem to affect the operation of the site on the client end.
The text was updated successfully, but these errors were encountered: