Prevent deadlock due to system call error #159
Conversation
The MQTT client runs in the main thread which consumes incoming MQTT messages from a `Thread::Queue` called `@read_queue`. This queue is fed by the `@read_thread`, a child thread which reads from a socket in an infinite loop. We noticed that our application stopped processing incoming MQTT messages, but did not seem to exit or throw an exception either. Upon inspecting the logs, we saw that the `@read_thread` had crashed due to an unhandled `Errno::ECONNRESET` while reading from the socket. This had the consuming thread then sleep forever while waiting for new meassages on the `@read_queue`. It turned out that the `MQTT::Client#receive_packet` method had appropriate error handling in place ```ruby def receive_packet # ... rescue Exception # ... end ``` but this did not rescue `Errno::ECONNRESET` even though `Exception` is at the top of the hierarchy of [Ruby's built-in exception classes][builtin-exceptions]. The root cause was that the library also defines a class `MQTT::Exception` and in the context of `#receive_packet` the constant `Exception` refers only to `MQTT::Exception` (which does not cover `Errno::ECONNRESET`) where it actually should rescue any subclass of `::Exception`. [builtin-exceptions]: https://docs.ruby-lang.org/en/3.2/Exception.html#class-Exception-label-Built-In+Exception+Classes
|
@phlegx FYI |
|
I'm running into a threaded processing lockup that seems to be a similar issue |
|
Any follow up on getting this merged? This was a real problem for us a few months ago. We found setting |
|
Thanks for this. The exception code has been wrong for a very long time and it is long overdue getting sorted out. According to Semver, do you think this change is worthy of a Major, Minor or Patch increment? 🤔 |
|
Hey @njh, good to see this progressing!
Since you're just merging fixes (I assume) I'd argue that is only a PATCH increment. If you're also taking #112, that one mandates a cautionary note in |
The MQTT client runs in the main thread which consumes incoming MQTT messages from a
Thread::Queuecalled@read_queue. This queue is fed by the@read_thread, a child thread which reads from a socket in an infinite loop.We noticed that our application stopped processing incoming MQTT messages, but did not seem to exit or throw an exception either. Upon inspecting the logs, we saw that the
@read_threadhad crashed due to an unhandledErrno::ECONNRESETwhile reading from the socket. This had the consuming thread then sleep forever while waiting for new meassages on the@read_queue.It turned out that the
MQTT::Client#receive_packetmethod had appropriate error handling in placebut this did not rescue
Errno::ECONNRESETeven thoughExceptionis at the top of the hierarchy ofRuby's built-in exception classes.
The root cause was that the library also defines a class
MQTT::Exceptionand in the context of#receive_packetthe constantExceptionrefers only toMQTT::Exception(which does not coverErrno::ECONNRESET) where it actually should rescue any subclass of::Exception.