🧵🥅 Reraise receiver thread errors with caller's backtrace

lib/net/imap.rb

@@ -3212,7 +3212,7 @@ def idle(timeout = nil, &response_handler)
           @idle_done_cond.wait(timeout)
           @idle_done_cond = nil
           if @receiver_thread_terminating
-            raise @exception || Net::IMAP::Error.new("connection closed")
+            reraise @exception || Net::IMAP::Error.new("connection closed")
           end
         ensure
           remove_response_handler(response_handler)
@@ -3587,34 +3587,6 @@ def receive_responses
       state_logout!
     end
 
-    def get_tagged_response(tag, cmd, timeout = nil)
-      if timeout
-        deadline = Time.now + timeout
-      end
-      until @tagged_responses.key?(tag)
-        raise @exception if @exception
-        if timeout
-          timeout = deadline - Time.now
-          if timeout <= 0
-            return nil
-          end
-        end
-        @tagged_response_arrival.wait(timeout)
-      end
-      resp = @tagged_responses.delete(tag)
-      case resp.name
-      when /\A(?:OK)\z/ni
-        return resp
-      when /\A(?:NO)\z/ni
-        raise NoResponseError, resp
-      when /\A(?:BAD)\z/ni
-        raise BadResponseError, resp
-      else
-        disconnect
-        raise InvalidResponseError, "invalid tagged resp: %p" % [resp.raw_data.chomp]
-      end
-    end
-
     def get_response
       buff = @reader.read_response_buffer
       return nil if buff.length == 0
@@ -3695,6 +3667,66 @@ def finish_sending_command(command)
       end
     end
 
+    def get_tagged_response(tag, cmd, timeout = nil)
+      if timeout
+        deadline = Time.now + timeout
+      end
+      until @tagged_responses.key?(tag)
+        reraise @exception
+        if timeout
+          timeout = deadline - Time.now
+          if timeout <= 0
+            return nil
+          end
+        end
+        @tagged_response_arrival.wait(timeout)
+      end
+      resp = @tagged_responses.delete(tag)
+      case resp.name
+      when /\A(?:OK)\z/ni
+        return resp
+      when /\A(?:NO)\z/ni
+        raise NoResponseError, resp
+      when /\A(?:BAD)\z/ni
+        raise BadResponseError, resp
+      else
+        disconnect
+        raise InvalidResponseError, "invalid tagged resp: %p" % [resp.raw_data.chomp]
+      end
+    end
+
+    # Raises a copy of +exception+ with an updated +backtrace+ and +cause+, or
+    # returns +nil+ when +exception+ is falsey.
+    #
+    # +backtrace+ is set to reflect the current caller.
+    #
+    # +cause+ is set to the original exception, _if_ the original has its own
+    # backtrace or cause.  Otherwise, the original was never raised and the
+    # default cause is used.
+    #
+    # This is meant for exceptions that may have been created by another thread.
+    # Raising them directly gives a misleading backtrace, making it hard to
+    # discover where the errors originate.  Although it is sometimes more
+    # appropriate to raise a wrapping exception, that changes the API and forces
+    # updates to users' `rescue` pattern matching.
+    def reraise(exception)
+      return unless exception
+      copy = exception.dup
+      # NOTE: set_backtrace(caller_locations) doesn't work for CRuby <= 3.3.
+      #   and set_backtrace(nil) doesn't work for TruffleRuby 34.0.0:
+      #   https://github.com/truffleruby/truffleruby/issues/4296
+      if RUBY_ENGINE == "truffleruby"
+        copy.set_backtrace caller_locations
+      else
+        copy.set_backtrace nil
+      end
+      if exception.cause || exception.backtrace
+        raise copy, cause: exception
+      else
+        raise copy
+      end
+    end
+
     def generate_tag
       @tagno += 1
       return format("%s%04d", @tag_prefix, @tagno)

lib/net/imap/command_data.rb

@@ -102,8 +102,7 @@ def send_literal(str, tag = nil, binary: false, non_sync: nil)
         @continuation_request_exception = nil
         begin
           @continuation_request_arrival.wait
-          e = @continuation_request_exception || @exception
-          raise e if e
+          reraise @continuation_request_exception || @exception
           put_string(str)
         ensure
           @continued_command_tag = nil

test/lib/helper.rb

@@ -190,6 +190,33 @@ def assert_stream_closed_error
     end
   end
 
+  # Combines +assert_raise+ and +assert_raise_with_message+ with an assertion
+  # that the backtrace matches the caller.
+  def assert_local_raise(expected, message = nil)
+    error = nil
+    block = -> do
+      yield
+    rescue expected => error
+      raise
+    end
+    if message
+      assert_raise_with_message(expected, message, &block)
+    else
+      assert_raise(expected, &block)
+    end
+    stack = caller
+    assert_equal stack, error.backtrace&.last(stack.size)
+    error
+  end
+
+  # Combines +assert_local_raise+ with an assertion that the exception's cause
+  # is in the receiver thread.
+  def assert_reraised(*args, imap: nil, &block)
+    cause = imap.instance_variable_get(:@receiver_thread).backtrace&.last(1)
+    error = assert_local_raise(*args, &block)
+    assert_equal cause, error.cause&.backtrace&.last(cause.size)
+  end
+
   def pend_if(condition, *args, &block)
     if condition
       pend(*args, &block)

test/net/imap/fake_server/command_reader.rb

@@ -42,6 +42,9 @@ def get_command
         throw :eof
       end
       raise
+    rescue Errno::EPIPE, Errno::ECONNRESET
+      throw :eof if config.ignore_abrupt_eof?
+      raise
     end
 
     private

test/net/imap/fake_server/connection.rb

@@ -42,6 +42,8 @@ def close
         end
         socket&.close unless socket&.closed?
       end
+    rescue Errno::EPIPE, Errno::ECONNRESET
+      raise unless config.ignore_abrupt_eof?
     end
 
     private

test/net/imap/test_imap.rb

@@ -13,7 +13,7 @@ class IMAPTest < Net::IMAP::TestCase
 
   if defined?(OpenSSL::SSL::SSLError)
     def test_imaps_unknown_ca
-      assert_raise(OpenSSL::SSL::SSLError) do
+      assert_local_raise(OpenSSL::SSL::SSLError) do
         imaps_test do |port|
           begin
             Net::IMAP.new("localhost",
@@ -81,7 +81,7 @@ def test_imaps_verify_none
     end
 
     def test_imaps_post_connection_check
-      assert_raise(OpenSSL::SSL::SSLError) do
+      assert_local_raise(OpenSSL::SSL::SSLError) do
         imaps_test do |port|
           # server_addr is different from the hostname in the certificate,
           # so the following code should raise a SSLError.
@@ -156,7 +156,7 @@ def test_starttls_stripping_not_ok
       end
       begin
         imap = Net::IMAP.new("localhost", :port => port)
-        assert_raise(Net::IMAP::InvalidResponseError) do
+        assert_reraised(Net::IMAP::InvalidResponseError, imap:) do
           imap.starttls(:ca_file => CA_FILE)
         end
         assert imap.disconnected?
@@ -195,7 +195,7 @@ def test_starttls_stripping_ok_sent_before_response
         client_to_server << :send_malicious_response
         assert_equal :malicious_response_sent, server_to_client.pop
         sleep 0.010 # to be sure the network buffers have flushed, etc
-        assert_raise(Net::IMAP::InvalidTaggedResponseError) do
+        assert_local_raise(Net::IMAP::InvalidTaggedResponseError) do
           imap.starttls(:ca_file => CA_FILE)
         end
         assert imap.disconnected?
@@ -250,7 +250,9 @@ def test_starttls_stripping_ok_sent_before_response
         assert_equal :sent_malicious_responses, server_to_client.pop
         assert_equal [1, 2, 3], 3.times.map { rcvr_to_client.pop }
         # should respond this way for _any_ command
-        assert_raise(Net::IMAP::InvalidTaggedResponseError) do cmd.(imap) end
+        assert_local_raise(Net::IMAP::InvalidTaggedResponseError) do
+          cmd.(imap)
+        end
         assert imap.disconnected?
         assert_stream_closed_error do cmd.(imap) end
         assert_stream_closed_error do cmd.(imap) end
@@ -286,14 +288,53 @@ def test_unexpected_eof
     end
     begin
       imap = Net::IMAP.new(server_addr, :port => port)
-      assert_raise(EOFError) do
+      assert_local_raise EOFError do
         imap.logout
       end
     ensure
       imap.disconnect if imap
     end
   end
 
+  test "exception from response reader" do
+    with_fake_server ignore_io_error: true, ignore_abrupt_eof: true do |server, imap|
+      imap.instance_exec do
+        def @reader.read_response_buffer
+          raise StandardError, "crashy crash"
+        end
+      end
+      assert_reraised(StandardError, imap:) do
+        imap.noop
+        # response to the first one might be okay,
+        # but the second attempt should definitely fail
+        imap.noop
+      end
+      assert imap.disconnected?
+    end
+  end
+
+  test "exception from response parser" do
+    with_fake_server ignore_io_error: true, ignore_abrupt_eof: true do |server, imap|
+      server.on "NOOP" do |resp|
+        resp.puts "#{resp.tag} NOPE [SERVERBUG] this ain't right!"
+      end
+      assert_reraised(Net::IMAP::InvalidResponseError, /bad.*NOPE/, imap:) do
+        imap.noop
+      end
+      assert imap.disconnected?
+    end
+    with_fake_server ignore_abrupt_eof: true do |server, imap|
+      server.on "FETCH" do |resp|
+        resp.untagged '1 FETCH (BODY[] ")'
+        resp.done_ok
+      end
+      assert_reraised(Net::IMAP::ResponseParseError, imap:) do
+        imap.fetch 1, "FAST"
+      end
+      assert imap.disconnected?
+    end
+  end
+
   def test_idle
     server = create_tcp_server
     port = server.addr[1]
@@ -420,7 +461,7 @@ def test_idle_done_not_during_idle
     end
     begin
       imap = Net::IMAP.new(server_addr, :port => port)
-      assert_raise(Net::IMAP::Error) do
+      assert_local_raise(Net::IMAP::Error) do
         imap.idle_done
       end
     ensure
@@ -501,7 +542,7 @@ def test_unexpected_bye
     end
     begin
       imap = Net::IMAP.new(server_addr, :port => port)
-      assert_raise(Net::IMAP::ByeResponseError) do
+      assert_local_raise(Net::IMAP::ByeResponseError) do
         imap.login("user", "password")
       end
     end
@@ -533,7 +574,7 @@ def @sock.shutdown(*args)
       end
       imap.logout
     ensure
-      assert_raise(RuntimeError) do
+      assert_local_raise(RuntimeError) do
         imap.disconnect
       end
     end
@@ -578,7 +619,7 @@ def test_connection_closed_during_idle
             c.signal
           end
         end
-        assert_raise(EOFError) do
+        assert_local_raise(EOFError) do
           imap.idle do |res|
             m.synchronize do
               in_idle = true
@@ -654,7 +695,7 @@ def tcp_socket(host, port)
         server.close
       end
     end
-    assert_raise(Net::IMAP::Error) do
+    assert_local_raise(Net::IMAP::Error) do
       #Net::IMAP.new(server_addr, :port => port)
       if true
           net_imap.new(server_addr, :port => port)
@@ -873,7 +914,7 @@ def test_raw_data
 
       # limited automatic non-synchronizing literals
       imap.config.max_non_synchronizing_literal = 5
-      assert_raise(Net::IMAP::NoResponseError) do
+      assert_local_raise(Net::IMAP::NoResponseError) do
         send_args.call [
           Net::IMAP::Literal["\rhi\r"],
           Net::IMAP::Literal["\x01" * 10],
@@ -1026,6 +1067,7 @@ def imaps_test(timeout: 10)
             sock.close
           end
         rescue Errno::EPIPE, Errno::ECONNRESET, Errno::ECONNABORTED
+        rescue OpenSSL::SSL::SSLError
         end
       end
       sleep 0.1 until started

test/net/imap/test_imap_authenticate.rb

@@ -209,7 +209,7 @@ class IMAPAuthenticateTest < Net::IMAP::TestCase
         server.state.authenticate(server.config.user)
         cmd.done_ok
       end
-      assert_raise(Net::IMAP::SASL::AuthenticationIncomplete) do
+      assert_local_raise(Net::IMAP::SASL::AuthenticationIncomplete) do
         imap.authenticate("DIGEST-MD5", "test_user", "test-password",
                           warn_deprecation: false)
       end

test/net/imap/test_imap_max_response_size.rb

@@ -22,7 +22,7 @@ class IMAPMaxResponseSizeTest < Net::IMAP::TestCase
   test "#max_response_size closes connection for too long line" do
     Net::IMAP.config.max_response_size = 10
     run_fake_server_in_thread(preauth: false, ignore_io_error: true) do |server|
-      assert_raise_with_message(
+      assert_local_raise(
         Net::IMAP::ResponseTooLargeError, /exceeds max_response_size .*\b10B\b/
       ) do
         with_client("localhost", port: server.port) do
@@ -40,9 +40,10 @@ class IMAPMaxResponseSizeTest < Net::IMAP::TestCase
       server.on("NOOP") do |resp|
         resp.untagged("1 FETCH (BODY[] {1000}\r\n" + "a" * 1000 + ")")
       end
-      assert_raise_with_message(
+      assert_reraised(
         Net::IMAP::ResponseTooLargeError,
-        /\d+B read \+ 1000B literal.* exceeds max_response_size .*\b50B\b/
+        /\d+B read \+ 1000B literal.* exceeds max_response_size .*\b50B\b/,
+        imap: client
       ) do
         client.noop
         fail "should not get here (FETCH literal longer than max_response_size)"

test/net/imap/test_imap_response_handlers.rb

@@ -72,4 +72,16 @@ class IMAPResponseHandlersTest < Net::IMAP::TestCase
     end
   end
 
+  test "error raised in response handler blocks later handlers" do
+    with_fake_server do |server, imap|
+      seen = []
+      imap.add_response_handler do seen << {first: _1.name} end
+      imap.add_response_handler do raise "ohno" end
+      imap.add_response_handler do seen << {last: _1.name} end
+
+      imap.noop
+      assert_equal [{first: "OK"}], seen
+    end
+  end
+
 end