Would make sense to have a "addChannelClosedHandler"?

[adinapoli]

Hi @hreinhardt ,

In production it would be nice to know and take action every time a channel is closed, for any reason. Since each channel connection is created in a separate thread, we are out of luck at the moment:

https://github.com/hreinhardt/amqp/blob/master/Network/AMQP/Internal.hs#L589

The library is rightfully using finally here, so all it would take is:

• A function very similar to addConnectionClosedHandler, which would allow us to register new handlers
• Call all the registered handler in the finally block, to make sure we get notified.

A perhaps even better solution would be to leverage forkFinally:

http://hackage.haskell.org/package/base-4.7.0.2/docs/Control-Concurrent.html#v:forkFinally

This would allow us to expose two new functions, addChannelClosedHandler and addChannelClosedExceptionHandler. Simply pattern matching on the result given back from forkFinally would allow us to call one group on handlers rather than the other. This would have the benefit to react differently whether the channel is closed "naturally" or if an exception caused it to be closed. Bear in mind thought that forkFinally is available from base 4.7 onwards, so a bit of CPP would be needed to support prior to 7.8.x

Do you think all of this would be an overkill?
Thanks a lot!

Alfredo

[hreinhardt]

Hi,

adding a function that's called for channel exceptions sounds like a good idea. But I'm not sure it makes sense to have another function for non-exceptional channel closes. AFAIK the only way a channel could be non-exceptionally closed is when the user calls closeChannel, in which case the user already knows the channel is about to be closed.

[adinapoli]

Sounds perfectly reasonable to me!
In case I might be able to carve some time this week and issue a PR, would you be keen on reviewing and eventually merging it?

Thanks for the blazing-fast reply ;)

[hreinhardt]

Yes, I'd review and most probably merge it.

[adinapoli]

Now that #53 has been merged, any chance for 0.12 to hit Hackage today? 😉

[hreinhardt]

Pushed it to Hackage. I initially wanted to also remove that catch in the channelReceiver but that seems to be a bit trickier than I expected.

Thank you for your contribution! Let's hope we didn't break anything.

[adinapoli]

Ha! Thank you very much! I am going to give a package a go next week, so if we did I will report back :)

[adinapoli]

As a small success story, our patch seems to work, at least superficially. This is what I get from the logs inside my development environment, when the server is running but I kill the broker abruptly:

"New job = ...
"New job = ...
Rebooting the server due to 127.0.0.1:5672: Network.Connection.connectionGet: end of file
Rebooting the server due to 127.0.0.1:5672: Network.Connection.connectionGet: end of file
Rebooting the server due to 127.0.0.1:5672: Network.Connection.connectionGet: end of file
Rebooting the server due to 127.0.0.1:5672: Network.Connection.connectionGet: end of file
Rebooting the server due to 127.0.0.1:5672: Network.Connection.connectionGet: end of file
Rebooting the server due to 127.0.0.1:5672: Network.Connection.connectionGet: end of file
Rebooting the server due to 127.0.0.1:5672: Network.Connection.connectionGet: end of file
Rebooting the server due to 127.0.0.1:5672: Network.Connection.connectionGet: end of file

the message that you see, is triggered by this simple helper function:

-- We explicitly filter out the two cases where a channel can be closed gracefully;
-- 1. When it received a ThreadKilled exception
-- 2. When a user is shutting it down as part of the normal connection closing routine.
reactToChannelException :: SomeException -> IO ()
reactToChannelException (asyncExceptionFromException -> Just ThreadKilled) = return ()
reactToChannelException (fromException -> Just (ConnectionClosedException "closed by user")) = return ()
reactToChannelException ex = do
  yellow $ T.pack $ "Rebooting the server due to " <> show ex
  rebootServer

(I'm using the ViewPattern extension to make everything a bit more slick).
More testing is certainly needed, but this looks to me like a promising start :)