Skip to content
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

Exception handling #24

Closed
wants to merge 13 commits into from
Closed

Conversation

thomasvnoort
Copy link
Contributor

I've made some improvements to the exception handling of the library:

  1. Unexpected situations should not write an error to stdout, instead, throw a custom exception or fail using error when it's really not supposed to happen. This allows users of the library to deal with such situations, instead of just seeing some message in their logs.
  2. Exceptions should not be caught and just written to stdout, for the same reasons as above.
  3. The handler that runs when a connection is closed could not distinguish between closings due to user request or an exception. I've made a simple change that records any exception and provides this to the handler so users of the library can distinguish the two cases. Note that this does change the interface of addConnectionClosedHandler. If desired, a wrapper function can be added to preserve the old interface. But, in general one wants to know why a connection was closed, so changing the interface is justifiable to me.

@fegu
Copy link
Contributor

fegu commented Sep 30, 2013

How will this address exceptions happening on the channel threads? I have used setUncaughtExceptionHandler in my code to catch those, and have seen this one: "recv: timeout (Operation timed out)" happening consistently when a stateful firewall closes the connection after a long period (2hours) of inactivity. It would be a real improvement if this instead went to the connectionClosedHandler.

@hreinhardt
Copy link
Owner

The recv exception most likely belonged to the connection itself and not to any one channel. This probably happened in an earlier version of the library, right? Because recently I switch from sockets to handles, so there's no call to recv anymore.
Hopefully we can solve these kinds of issues with heartbeating.

@fegu
Copy link
Contributor

fegu commented Oct 2, 2013

Yes, this was before the switch to handles. I just tested this again last night. In the most recent version nothing happens, that is, a client consumer just silently ceases to receive anything. No exceptions thrown. This is the worst of all possibilities. Heartbeats would solve this, but I feel the library should signal something as well. One way to test is simply to have a client connect to RabbitMQ, then yank out the network cable from your switch. As you can see, nothing happens on the client.
This is opposed to, say, logging in to RabbitMQ and forcibly closing the connection. In this case the client's connectionClosedHandled will be called.

@hreinhardt
Copy link
Owner

I think this behaviour (handle-read not returning on a closed connection) is caused by GHC so I'm not sure how we could fix this.

@thomasvnoort
Copy link
Contributor Author

Closing this PR since it's stale.

@thomasvnoort thomasvnoort deleted the exception-handling branch November 24, 2021 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants