Fix server cancel - Githubissues

Polyconseil / aioamqp

AMQP implementation using asyncio

Other

280 stars 88 forks source link

Fix server cancel #95

Closed foolswood closed 7 years ago

foolswood commented 8 years ago

It was causing a dispatch error.

foolswood commented 8 years ago

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.

foolswood commented 8 years ago

I'm hitting this quite frequently, and the diff is fairly small.

foolswood commented 8 years ago

I've been running in production with all 3 of these PRs for months now, nothing untoward seen.

RemiCardona commented 8 years ago

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.

RemiCardona commented 7 years ago

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.

foolswood commented 7 years ago

I've moved jobs and am no longer working with this daily so have no real world way to test.

RemiCardona commented 7 years ago

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.

RemiCardona commented 7 years ago

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.