💥 Changes to responses handling, motivated by thread-safety

lib/net/imap.rb

@@ -511,10 +511,12 @@ module Net
   #
   # - #greeting: The server's initial untagged response, which can indicate a
   #   pre-authenticated connection.
-  # - #responses: A hash with arrays of unhandled <em>non-+nil+</em>
-  #   UntaggedResponse and ResponseCode +#data+, keyed by +#name+.
+  # - #responses: Yields unhandled UntaggedResponse#data and <em>non-+nil+</em>
+  #   ResponseCode#data.
+  # - #clear_responses: Deletes unhandled data from #responses and returns it.
   # - #add_response_handler: Add a block to be called inside the receiver thread
   #   with every server response.
+  # - #response_handlers: Returns the list of response handlers.
   # - #remove_response_handler: Remove a previously added response handler.
   #
   #
@@ -710,22 +712,6 @@ class IMAP < Protocol
     # Returns the initial greeting the server, an UntaggedResponse.
     attr_reader :greeting
 
-    # Returns a hash with arrays of unhandled <em>non-+nil+</em>
-    # UntaggedResponse#data keyed by UntaggedResponse#name, and
-    # ResponseCode#data keyed by ResponseCode#name.
-    #
-    # For example:
-    #
-    #   imap.select("inbox")
-    #   p imap.responses["EXISTS"][-1]
-    #   #=> 2
-    #   p imap.responses["UIDVALIDITY"][-1]
-    #   #=> 968263756
-    attr_reader :responses
-
-    # Returns all response handlers.
-    attr_reader :response_handlers
-
     # Seconds to wait until a connection is opened.
     # If the IMAP object cannot open a connection within this time,
     # it raises a Net::OpenTimeout exception. The default value is 30 seconds.
@@ -734,8 +720,6 @@ class IMAP < Protocol
     # Seconds to wait until an IDLE response is received.
     attr_reader :idle_response_timeout
 
-    attr_accessor :client_thread # :nodoc:
-
     # The hostname this client connected to
     attr_reader :host
 
@@ -768,6 +752,11 @@ class << self
       alias default_ssl_port default_tls_port
     end
 
+    def client_thread # :nodoc:
+      warn "Net::IMAP#client_thread is deprecated and will be removed soon."
+      @client_thread
+    end
+
     # Disconnects from the server.
     #
     # Related: #logout
@@ -1084,11 +1073,11 @@ def login(user, password)
     # to select a +mailbox+ so that messages in the +mailbox+ can be accessed.
     #
     # After you have selected a mailbox, you may retrieve the number of items in
-    # that mailbox from <tt>imap.responses["EXISTS"][-1]</tt>, and the number of
-    # recent messages from <tt>imap.responses["RECENT"][-1]</tt>.  Note that
-    # these values can change if new messages arrive during a session or when
-    # existing messages are expunged; see #add_response_handler for a way to
-    # detect these events.
+    # that mailbox from <tt>imap.responses("EXISTS", &:last)</tt>, and the
+    # number of recent messages from <tt>imap.responses("RECENT", &:last)</tt>.
+    # Note that these values can change if new messages arrive during a session
+    # or when existing messages are expunged; see #add_response_handler for a
+    # way to detect these events.
     #
     # A Net::IMAP::NoResponseError is raised if the mailbox does not
     # exist or is for some reason non-selectable.
@@ -1954,6 +1943,104 @@ def idle_done
       end
     end
 
+    # :call-seq:
+    #   responses       {|hash|  ...} -> block result
+    #   responses(type) {|array| ...} -> block result
+    #
+    # Yields unhandled responses and returns the result of the block.
+    #
+    # Unhandled responses are stored in a hash, with arrays of
+    # <em>non-+nil+</em> UntaggedResponse#data keyed by UntaggedResponse#name
+    # and ResponseCode#data keyed by ResponseCode#name.  Call without +type+ to
+    # yield the entire responses hash.  Call with +type+ to yield only the array
+    # of responses for that type.
+    #
+    # For example:
+    #
+    #   imap.select("inbox")
+    #   p imap.responses("EXISTS", &:last)
+    #   #=> 2
+    #   p imap.responses("UIDVALIDITY", &:last)
+    #   #=> 968263756
+    #
+    # >>>
+    #   *Note:* Access to the responses hash is synchronized for thread-safety.
+    #   The receiver thread and response_handlers cannot process new responses
+    #   until the block completes.  Accessing either the response hash or its
+    #   response type arrays outside of the block is unsafe.
+    #
+    #   Calling without a block is unsafe and deprecated.  Future releases will
+    #   raise ArgumentError unless a block is given.
+    #
+    # Previously unhandled responses are automatically cleared before entering a
+    # mailbox with #select or #examine.  Long-lived connections can receive many
+    # unhandled server responses, which must be pruned or they will continually
+    # consume more memory.  Update or clear the responses hash or arrays inside
+    # the block, or use #clear_responses.
+    #
+    # Only non-+nil+ data is stored.  Many important response codes have no data
+    # of their own, but are used as "tags" on the ResponseText object they are
+    # attached to.  ResponseText will be accessible by its response types:
+    # "+OK+", "+NO+", "+BAD+", "+BYE+", or "+PREAUTH+".
+    #
+    # TaggedResponse#data is not saved to #responses, nor is any
+    # ResponseCode#data on tagged responses.  Although some command methods do
+    # return the TaggedResponse directly, #add_response_handler must be used to
+    # handle all response codes.
+    #
+    # Related: #clear_responses, #response_handlers, #greeting
+    def responses(type = nil)
+      if block_given?
+        synchronize { yield(type ? @responses[type.to_s.upcase] : @responses) }
+      elsif type
+        raise ArgumentError, "Pass a block or use #clear_responses"
+      else
+        # warn("DEPRECATED: pass a block or use #clear_responses", uplevel: 1)
+        @responses
+      end
+    end
+
+    # :call-seq:
+    #   clear_responses       -> hash
+    #   clear_responses(type) -> array
+    #
+    # Clears and returns the unhandled #responses hash or the unhandled
+    # responses array for a single response +type+.
+    #
+    # Clearing responses is synchronized with other threads.  The lock is
+    # released before returning.
+    #
+    # Related: #responses, #response_handlers
+    def clear_responses(type = nil)
+      synchronize {
+        if type
+          @responses.delete(type) || []
+        else
+          @responses.dup.transform_values(&:freeze)
+            .tap { _1.default = [].freeze }
+            .tap { @responses.clear }
+        end
+      }
+        .freeze
+    end
+
+    # Returns all response handlers, including those that are added internally
+    # by commands.  Each response handler will be called with every new
+    # UntaggedResponse, TaggedResponse, and ContinuationRequest.
+    #
+    # Response handlers are called with a mutex inside the receiver thread.  New
+    # responses cannot be processed and commands from other threads must wait
+    # until all response_handlers return.  An exception will shut-down the
+    # receiver thread and close the connection.
+    #
+    # For thread-safety, the returned array is a frozen copy of the internal
+    # array.
+    #
+    # Related: #add_response_handler, #remove_response_handler
+    def response_handlers
+      synchronize { @response_handlers.clone.freeze }
+    end
+
     # Adds a response handler. For example, to detect when
     # the server sends a new EXISTS response (which normally
     # indicates new messages being added to the mailbox),
@@ -1966,14 +2053,21 @@ def idle_done
     #     end
     #   }
     #
+    # Related: #remove_response_handler, #response_handlers
     def add_response_handler(handler = nil, &block)
       raise ArgumentError, "two Procs are passed" if handler && block
-      @response_handlers.push(block || handler)
+      synchronize do
+        @response_handlers.push(block || handler)
+      end
     end
 
     # Removes the response handler.
+    #
+    # Related: #add_response_handler, #response_handlers
     def remove_response_handler(handler)
-      @response_handlers.delete(handler)
+      synchronize do
+        @response_handlers.delete(handler)
+      end
     end
 
     private

test/net/imap/test_imap.rb

@@ -860,7 +860,7 @@ def test_uidplus_responses
       assert_equal([38505, [3955], [3967]], resp.data.code.data.to_a)
       imap.select('trash')
       assert_equal(
-        imap.responses["NO"].last.code,
+        imap.responses("NO", &:last).code,
         Net::IMAP::ResponseCode.new('UIDNOTSTICKY', nil)
       )
       imap.logout
@@ -870,37 +870,123 @@ def test_uidplus_responses
   end
 
   def yields_in_test_server_thread(
-    greeting = "* OK [CAPABILITY IMAP4rev1 AUTH=PLAIN STARTTLS] test server\r\n"
+    read_timeout: 2, # requires ruby 3.2+
+    timeout: 10,
+    greeting: "* OK [CAPABILITY IMAP4rev1 AUTH=PLAIN STARTTLS] test server\r\n"
   )
     server = create_tcp_server
     port   = server.addr[1]
+    last_tag, last_cmd, last_args = nil
     @threads << Thread.start do
-      sock = server.accept
-      gets = ->{
-        buf = "".b
-        buf << sock.gets until /\A([^ ]+) ([^ ]+) ?(.*)\r\n\z/mn =~ buf
-        [$1, $2, $3]
-      }
-      begin
-        sock.print(greeting)
-        last_tag = yield sock, gets
-        sock.print("* BYE terminating connection\r\n")
-        sock.print("#{last_tag} OK LOGOUT completed\r\n") if last_tag
-      ensure
-        sock.close
-        server.close
+      Timeout.timeout(timeout) do
+        sock = server.accept
+        sock.timeout = read_timeout if sock.respond_to? :timeout # ruby 3.2+
+        sock.singleton_class.define_method(:getcmd) do
+          buf = "".b
+          buf << (sock.gets || "") until /\A([^ ]+) ([^ ]+) ?(.*)\r\n\z/mn =~ buf
+          [last_tag = $1, last_cmd = $2, last_args = $3]
+        end
+        begin
+          sock.print(greeting)
+          yield sock
+        ensure
+          begin
+            sock.print("* BYE terminating connection\r\n")
+            last_cmd =~ /LOGOUT/i and
+              sock.print("#{last_tag} OK LOGOUT completed\r\n")
+          ensure
+            sock.close
+            server.close
+          end
+        end
       end
     end
     port
   end
 
+  # SELECT returns many different untagged results, so this is useful for
+  # several different tests.
+  RFC3501_6_3_1_SELECT_EXAMPLE_DATA = <<~RESPONSES
+    * 172 EXISTS
+    * 1 RECENT
+    * OK [UNSEEN 12] Message 12 is first unseen
+    * OK [UIDVALIDITY 3857529045] UIDs valid
+    * OK [UIDNEXT 4392] Predicted next UID
+    * FLAGS (\\Answered \\Flagged \\Deleted \\Seen \\Draft)
+    * OK [PERMANENTFLAGS (\\Deleted \\Seen \\*)] Limited
+    %{tag} OK [READ-WRITE] SELECT completed
+  RESPONSES
+    .split("\n").join("\r\n").concat("\r\n").freeze
+
+  def test_responses
+    port = yields_in_test_server_thread do |sock|
+      tag, name, = sock.getcmd
+      if name == "SELECT"
+        sock.print RFC3501_6_3_1_SELECT_EXAMPLE_DATA % {tag: tag}
+      end
+      sock.getcmd # waits for logout command
+    end
+    begin
+      imap = Net::IMAP.new(server_addr, port: port)
+      resp = imap.select "INBOX"
+      assert_equal([Net::IMAP::TaggedResponse, "RUBY0001", "OK"],
+                   [resp.class, resp.tag, resp.name])
+      assert_equal([172], imap.responses { _1["EXISTS"] })
+      assert_equal([3857529045], imap.responses("UIDVALIDITY") { _1 })
+      assert_equal(1, imap.responses("RECENT", &:last))
+      assert_raise(ArgumentError) do imap.responses("UIDNEXT") end
+      # Deprecated style, without a block:
+      # assert_warn(/Pass a block.*or.*clear_responses/i) do
+      #   assert_equal(%i[Answered Flagged Deleted Seen Draft],
+      #                imap.responses["FLAGS"]&.last)
+      # end
+      imap.logout
+    ensure
+      imap.disconnect if imap
+    end
+  end
+
+  def test_clear_responses
+    port = yields_in_test_server_thread do |sock|
+      tag, name, = sock.getcmd
+      if name == "SELECT"
+        sock.print RFC3501_6_3_1_SELECT_EXAMPLE_DATA % {tag: tag}
+      end
+      sock.getcmd # waits for logout command
+    end
+    begin
+      imap = Net::IMAP.new(server_addr, port: port)
+      resp = imap.select "INBOX"
+      assert_equal([Net::IMAP::TaggedResponse, "RUBY0001", "OK"],
+                   [resp.class, resp.tag, resp.name])
+      # called with "type", clears and returns only that type
+      assert_equal([172],        imap.clear_responses("EXISTS"))
+      assert_equal([],           imap.clear_responses("EXISTS"))
+      assert_equal([1],          imap.clear_responses("RECENT"))
+      assert_equal([3857529045], imap.clear_responses("UIDVALIDITY"))
+      # called without "type", clears and returns all responses
+      responses = imap.clear_responses
+      assert_equal([],   responses["EXISTS"])
+      assert_equal([],   responses["RECENT"])
+      assert_equal([],   responses["UIDVALIDITY"])
+      assert_equal([12], responses["UNSEEN"])
+      assert_equal([4392], responses["UIDNEXT"])
+      assert_equal(5, responses["FLAGS"].last&.size)
+      assert_equal(3, responses["PERMANENTFLAGS"].last&.size)
+      assert_equal({}, imap.responses(&:itself))
+      assert_equal({}, imap.clear_responses)
+      imap.logout
+    ensure
+      imap.disconnect if imap
+    end
+  end
+
   def test_close
     requests = Queue.new
-    port = yields_in_test_server_thread do |sock, gets|
-      requests.push(gets[])
+    port = yields_in_test_server_thread do |sock|
+      requests << sock.getcmd
       sock.print("RUBY0001 OK CLOSE completed\r\n")
-      requests.push(gets[])
-      "RUBY0002"
+      requests << sock.getcmd
     end
     begin
       imap = Net::IMAP.new(server_addr, :port => port)
@@ -917,14 +1003,13 @@ def test_close
 
   def test_unselect
     requests = Queue.new
-    port = yields_in_test_server_thread do |sock, gets|
-      requests.push(gets[])
+    port = yields_in_test_server_thread do |sock|
+      requests << sock.getcmd
       sock.print("RUBY0001 OK UNSELECT completed\r\n")
-      requests.push(gets[])
-      "RUBY0002"
+      requests << sock.getcmd
     end
     begin
-      imap = Net::IMAP.new(server_addr, :port => port)
+      imap = Net::IMAP.new(server_addr, port: port)
       resp = imap.unselect
       assert_equal(["RUBY0001", "UNSELECT", ""], requests.pop)
       assert_equal([Net::IMAP::TaggedResponse, "RUBY0001", "OK"],