hreinhardt/amqp

`ackEnv envelope` kills query consumer when channel is in `NoAck` mode

Opened this issue · 10 comments

witoldsz commented

In my app, by mistake, I did manually ackEnv a message received by a channel in the NoAck mode. The application did not crash, but the query consumer was silently dropped. It took me an hour to figure out what had happened. Is this a bug?

I am using Stack resolver lts-9.2, so I am at amqp-0.15.1.

michaelklishin commented

This is not a bug. Acknowledging with an unknown delivery tag results in a channel error which closes the channel. There can be no deliveries on a closed channel and all of its state is discarded.

michaelklishin commented

Also notifications about channel errors are asynchronous so clients cannot immediately throw an exception when a basic.ack is sent.

hreinhardt commented

You can use addChannelExceptionHandler to be notified whenever a channel is unexpectedly closed. In your case you would have got something like ConnectionClosedException "PRECONDITION_FAILED - unknown delivery tag 2". (Not sure why the error says ConnectionClosed instead of ChannelClosed; could be a bug in the ampq library. I'll take a look.)

I guess you weren't aware this function existed, so maybe there's a way we could make it more prominent.

hreinhardt commented

I added a "debugging tips" section to the docs in the newest release, to help people that have to debug similar problems in the future.

  • 👍1
witoldsz commented

Thanks for clarification. That would be nice if such errors could by, by default, displayed to STDERR until addXyzExceptionHandler is registered. That would be best of both worlds: we can decide what to do with exceptions and at the same time we would be sure nothing gets wrong without a notice.

  • 👍1
hreinhardt commented

While I personally agree with you, in the past people have complained about the library printing stuff to the screen unprompted. So no matter what we do, we will annoy some group of people...

  • 👍1
michaelklishin commented

@hreinhardt in the age of logging to stdout/stderr required by some platforms I think it's more acceptable to do that. Also, when I have to debug something I personally would rather have more information about errors and suspicious events, not less.

witoldsz commented

in the past people have complained about the library printing stuff to the screen unprompted

This is why I suggest that printing would be just the default action until some exception handler is registered. Once it's registered the library user would decide for themselves what to do (to log it or not, if so then how).

hreinhardt commented

OK, your arguments are definitely valid.

I think I have a rough idea how this could be implemented and I'll give it a try.

  • 👍1
hreinhardt commented

I've pushed the implementation to master: 3363031

It seems to work in my very limited testing, but would be great if somebody else could give it a try.