Fix server cancel #95
Conversation
the old test didn't actually cause a cancel, which would have caused a dispatch error, which it also wouldn't have detected.
|
Might be worth bringing the dispatch error thing up higher in the tests, but I suspect there will be a larger set of changes involved, so I thought I'd try this first. |
|
I'm hitting this quite frequently, and the diff is fairly small. |
|
I've been running in production with all 3 of these PRs for months now, nothing untoward seen. |
|
So we got this traceback on our Sentry over the weekend. KeyError: 'consumer_tag'
File "aioamqp/protocol.py", line 262, in run
yield from self.dispatch_frame()
File "aioamqp/protocol.py", line 217, in dispatch_frame
yield from channel.dispatch_frame(frame)
File "aioamqp/channel.py", line 111, in dispatch_frame
yield from methods[(frame.class_id, frame.method_id)](frame)
File "asyncio/coroutines.py", line 141, in coro
res = func(*args, **kw)
File "aioamqp/channel.py", line 665, in server_basic_cancel
consumer_tag = frame.arguments['consumer_tag']Really looks like this PR solves this problem. I had already started looking at open PRs but this one just got bumped to the top of the queue. Thanks for your patience. |
The old test didn't actually cause a cancel, which would have caused a dispatch error.
|
Alright! So I took a swing at this, thinking I could put this in the upcoming release, and the https://github.com/Polyconseil/aioamqp/tree/rcardona_pr95 branch is a start. But digging into it, I think the whole cancel code needs to be looked at: cleaning up old callbacks, reusing old consumer tags, basically making sure the client-initiated and server initiated cancels work the same way. So I don't think this is worth merging yet, as I need to cut a release today. I'm definitely keeping this at the top of the todo because I still see this error on our production servers every now and then. Thanks again for the patch and your patience. |
|
I've moved jobs and am no longer working with this daily so have no real world way to test. |
|
Reopening because it's still an issue. @foolswood thanks for your help debugging this and providing us with this PR. Sorry we couldn't act on it before you moved on. |
|
Alright, so I've pulled this patch in with just a few minor changes because It Helps™. It's not perfect but it does improve the situation by reducing useless noise. The bigger changes — actually doing something with the server-initiated cancel — I mentioned a while back will be done as part of the API redesign in #118. Thanks again for the contribution. |
It was causing a dispatch error.