From 0b967ff6177cce139d6054df8af2ee6ea793a5db Mon Sep 17 00:00:00 2001
From: Leo Arnold <leoarnold@users.noreply.github.com>
Date: Wed, 3 Apr 2024 00:53:56 +0200
Subject: [PATCH] Prevent deadlock due to system call error (#159)

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
---
 lib/mqtt/client.rb       |  2 +-
 spec/mqtt_client_spec.rb | 20 +++++++++++++++++---
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/lib/mqtt/client.rb b/lib/mqtt/client.rb
index 8b81ebf..3d1849d 100644
--- a/lib/mqtt/client.rb
+++ b/lib/mqtt/client.rb
@@ -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
diff --git a/spec/mqtt_client_spec.rb b/spec/mqtt_client_spec.rb
index 5ae92ab..f7604d9 100644
--- a/spec/mqtt_client_spec.rb
+++ b/spec/mqtt_client_spec.rb
@@ -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