Skip to content

Commit

Permalink
Prevent deadlock due to system call error (#159)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
leoarnold authored Apr 2, 2024
1 parent 271ee63 commit 0b967ff
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 4 deletions.
2 changes: 1 addition & 1 deletion lib/mqtt/client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,7 @@ def receive_packet
end
keep_alive!
# Pass exceptions up to parent thread
rescue Exception => exp
rescue ::Exception => exp
unless @socket.nil?
@socket.close
@socket = nil
Expand Down
20 changes: 17 additions & 3 deletions spec/mqtt_client_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -923,15 +923,29 @@ def wait_for_puback(id, queue)
expect(@read_queue.size).to eq(0)
end

it "should close the socket if there is an exception" do
it "should close the socket if there is an MQTT exception" do
expect(socket).to receive(:close).once
allow(MQTT::Packet).to receive(:read).and_raise(MQTT::Exception)
client.send(:receive_packet)
end

it "should close the socket if there is a system call error" do
expect(socket).to receive(:close).once
allow(MQTT::Packet).to receive(:read).and_raise(Errno::ECONNRESET)
client.send(:receive_packet)
end

it "should pass exceptions up to parent thread" do
expect(@parent_thread).to receive(:raise).once
allow(MQTT::Packet).to receive(:read).and_raise(MQTT::Exception)
e = MQTT::Exception.new
expect(@parent_thread).to receive(:raise).with(e).once
allow(MQTT::Packet).to receive(:read).and_raise(e)
client.send(:receive_packet)
end

it "should pass a system call error up to parent thread" do
e = Errno::ECONNRESET.new
expect(@parent_thread).to receive(:raise).with(e).once
allow(MQTT::Packet).to receive(:read).and_raise(e)
client.send(:receive_packet)
end

Expand Down

0 comments on commit 0b967ff

Please sign in to comment.