🔀 Merge pull request #334 from ruby/responses-return-frozen_dup

nevans · web-flow · commit 0f7d264fc344 · 2024-10-12T22:47:39.000-04:00
✨ New config option to return frozen dup from `#responses`
diff --git a/lib/net/imap.rb b/lib/net/imap.rb
@@ -2490,41 +2490,98 @@ def idle_done
       end
     end
 
+    RESPONSES_DEPRECATION_MSG =
+      "Pass a type or block to #responses, " \
+      "set config.responses_without_block to :frozen_dup " \
+      "or :silence_deprecation_warning, " \
+      "or use #extract_responses or #clear_responses."
+    private_constant :RESPONSES_DEPRECATION_MSG
+
     # :call-seq:
+    #   responses       -> hash of {String => Array} (see config.responses_without_block)
+    #   responses(type) -> frozen array
     #   responses       {|hash|  ...} -> block result
     #   responses(type) {|array| ...} -> block result
     #
-    # Yields unhandled responses and returns the result of the block.
+    # Yields or returns unhandled server responses.  Unhandled responses are
+    # stored in a hash, with arrays of UntaggedResponse#data keyed by
+    # UntaggedResponse#name and <em>non-+nil+</em> untagged ResponseCode#data
+    # keyed by ResponseCode#name.
+    #
+    # When a block is given, yields unhandled responses and returns the block's
+    # result.  Without a block, returns the unhandled responses.
+    #
+    # [With +type+]
+    #   Yield or return only the array of responses for that +type+.
+    #   When no block is given, the returned array is a frozen copy.
+    # [Without +type+]
+    #   Yield or return the entire responses hash.
+    #
+    #   When no block is given, the behavior is determined by
+    #   Config#responses_without_block:
+    #   >>>
+    #     [+:silence_deprecation_warning+ <em>(original behavior)</em>]
+    #       Returns the mutable responses hash (without any warnings).
+    #       <em>This is not thread-safe.</em>
+    #
+    #     [+:warn+ <em>(default since +v0.5+)</em>]
+    #       Prints a warning and returns the mutable responses hash.
+    #       <em>This is not thread-safe.</em>
+    #
+    #     [+:frozen_dup+ <em>(planned default for +v0.6+)</em>]
+    #       Returns a frozen copy of the unhandled responses hash, with frozen
+    #       array values.
     #
-    # 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.
+    #     [+:raise+]
+    #       Raise an +ArgumentError+ with the deprecation warning.
     #
     # For example:
     #
     #   imap.select("inbox")
-    #   p imap.responses("EXISTS", &:last)
+    #   p imap.responses("EXISTS").last
     #   #=> 2
+    #   p imap.responses("UIDNEXT", &:last)
+    #   #=> 123456
     #   p imap.responses("UIDVALIDITY", &:last)
     #   #=> 968263756
+    #   p imap.responses {|responses|
+    #     {
+    #       exists:      responses.delete("EXISTS").last,
+    #       uidnext:     responses.delete("UIDNEXT").last,
+    #       uidvalidity: responses.delete("UIDVALIDITY").last,
+    #     }
+    #   }
+    #   #=> {:exists=>2, :uidnext=>123456, :uidvalidity=>968263756}
+    #   # "EXISTS", "UIDNEXT", and "UIDVALIDITY" have been removed:
+    #   p imap.responses(&:keys)
+    #   #=> ["FLAGS", "OK", "PERMANENTFLAGS", "RECENT", "HIGHESTMODSEQ"]
+    #
+    # Related: #extract_responses, #clear_responses, #response_handlers, #greeting
     #
+    # ===== Thread safety
     # >>>
     #   *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.
+    #   response type arrays outside of the block is unsafe.  They can be safely
+    #   updated inside the block.  Consider using #clear_responses or
+    #   #extract_responses instead.
+    #
+    #   Net::IMAP will add and remove responses from the responses hash and its
+    #   array values, in the calling threads for commands and in the receiver
+    #   thread, but will not modify any responses after adding them to the
+    #   responses hash.
     #
-    #   Calling without a block is unsafe and deprecated.  Future releases will
-    #   raise ArgumentError unless a block is given.
-    #   See Config#responses_without_block.
+    # ===== Clearing responses
     #
     # 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.
+    # the block, or remove responses with #extract_responses, #clear_responses,
+    # or #add_response_handler.
+    #
+    # ===== Missing 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
@@ -2535,20 +2592,24 @@ def idle_done
     # 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: #extract_responses, #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"
+        synchronize { @responses[type.to_s.upcase].dup.freeze }
       else
         case config.responses_without_block
         when :raise
-          raise ArgumentError, "Pass a block or use #clear_responses"
+          raise ArgumentError, RESPONSES_DEPRECATION_MSG
         when :warn
-          warn("DEPRECATED: pass a block or use #clear_responses",
-               uplevel: 1, category: :deprecated)
+          warn(RESPONSES_DEPRECATION_MSG, uplevel: 1, category: :deprecated)
+        when :frozen_dup
+          synchronize {
+            responses = @responses.transform_values(&:freeze)
+            responses.default_proc = nil
+            responses.default = [].freeze
+            return responses.freeze
+          }
         end
         @responses
       end
diff --git a/lib/net/imap/config.rb b/lib/net/imap/config.rb
@@ -7,10 +7,10 @@
 module Net
   class IMAP
 
-    # Net::IMAP::Config stores configuration options for Net::IMAP clients.
-    # The global configuration can be seen at either Net::IMAP.config or
-    # Net::IMAP::Config.global, and the client-specific configuration can be
-    # seen at Net::IMAP#config.
+    # Net::IMAP::Config <em>(available since +v0.4.13+)</em> stores
+    # configuration options for Net::IMAP clients.  The global configuration can
+    # be seen at either Net::IMAP.config or Net::IMAP::Config.global, and the
+    # client-specific configuration can be seen at Net::IMAP#config.
     #
     # When creating a new client, all unhandled keyword arguments to
     # Net::IMAP.new are delegated to Config.new.  Every client has its own
@@ -128,7 +128,7 @@ def self.default; @default end
       # The global config object.  Also available from Net::IMAP.config.
       def self.global; @global if defined?(@global) end
 
-      # A hash of hard-coded configurations, indexed by version number.
+      # A hash of hard-coded configurations, indexed by version number or name.
       def self.version_defaults; @version_defaults end
       @version_defaults = {}
 
@@ -172,9 +172,16 @@ def self.[](config)
       include AttrInheritance
       include AttrTypeCoercion
 
-      # The debug mode (boolean)
+      # The debug mode (boolean).  The default value is +false+.
       #
-      # The default value is +false+.
+      # When #debug is +true+:
+      # * Data sent to and received from the server will be logged.
+      # * ResponseParser will print warnings with extra detail for parse
+      #   errors.  _This may include recoverable errors._
+      # * ResponseParser makes extra assertions.
+      #
+      # *NOTE:* Versioned default configs inherit #debug from Config.global, and
+      # #load_defaults will not override #debug.
       attr_accessor :debug, type: :boolean
 
       # method: debug?
@@ -200,60 +207,84 @@ def self.[](config)
       # The default value is +5+ seconds.
       attr_accessor :idle_response_timeout, type: Integer
 
-      # :markup: markdown
-      #
       # Whether to use the +SASL-IR+ extension when the server and \SASL
-      # mechanism both support it.
+      # mechanism both support it.  Can be overridden by the +sasl_ir+ keyword
+      # parameter to Net::IMAP#authenticate.
+      #
+      # <em>(Support for +SASL-IR+ was added in +v0.4.0+.)</em>
       #
-      # See Net::IMAP#authenticate.
+      # ==== Valid options
       #
-      # | Starting with version | The default value is                     |
-      # |-----------------------|------------------------------------------|
-      # | _original_            | +false+ <em>(extension unsupported)</em> |
-      # | v0.4                  | +true+  <em>(support added)</em>         |
+      # [+false+ <em>(original behavior, before support was added)</em>]
+      #   Do not use +SASL-IR+, even when it is supported by the server and the
+      #   mechanism.
+      #
+      # [+true+ <em>(default since +v0.4+)</em>]
+      #   Use +SASL-IR+ when it is supported by the server and the mechanism.
       attr_accessor :sasl_ir, type: :boolean
 
-      # :markup: markdown
-      #
-      # Controls the behavior of Net::IMAP#login when the `LOGINDISABLED`
+      # Controls the behavior of Net::IMAP#login when the +LOGINDISABLED+
       # capability is present.  When enforced, Net::IMAP will raise a
-      # LoginDisabledError when that capability is present.  Valid values are:
+      # LoginDisabledError when that capability is present.
       #
-      # [+false+]
+      # <em>(Support for +LOGINDISABLED+ was added in +v0.5.0+.)</em>
+      #
+      # ==== Valid options
+      #
+      # [+false+ <em>(original behavior, before support was added)</em>]
       #   Send the +LOGIN+ command without checking for +LOGINDISABLED+.
       #
       # [+:when_capabilities_cached+]
       #   Enforce the requirement when Net::IMAP#capabilities_cached? is true,
       #   but do not send a +CAPABILITY+ command to discover the capabilities.
       #
-      # [+true+]
+      # [+true+ <em>(default since +v0.5+)</em>]
       #   Only send the +LOGIN+ command if the +LOGINDISABLED+ capability is not
       #   present.  When capabilities are unknown, Net::IMAP will automatically
       #   send a +CAPABILITY+ command first before sending +LOGIN+.
       #
-      # | Starting with version   | The default value is           |
-      # |-------------------------|--------------------------------|
-      # | _original_              | `false`                        |
-      # | v0.5                    | `true`                         |
       attr_accessor :enforce_logindisabled, type: [
         false, :when_capabilities_cached, true
       ]
 
-      # :markup: markdown
+      # Controls the behavior of Net::IMAP#responses when called without any
+      # arguments (+type+ or +block+).
+      #
+      # ==== Valid options
+      #
+      # [+:silence_deprecation_warning+ <em>(original behavior)</em>]
+      #   Returns the mutable responses hash (without any warnings).
+      #   <em>This is not thread-safe.</em>
+      #
+      # [+:warn+ <em>(default since +v0.5+)</em>]
+      #   Prints a warning and returns the mutable responses hash.
+      #   <em>This is not thread-safe.</em>
       #
-      # Controls the behavior of Net::IMAP#responses when called without a
-      # block.  Valid options are `:warn`, `:raise`, or
-      # `:silence_deprecation_warning`.
+      # [+:frozen_dup+ <em>(planned default for +v0.6+)</em>]
+      #   Returns a frozen copy of the unhandled responses hash, with frozen
+      #   array values.
       #
-      # | Starting with version   | The default value is           |
-      # |-------------------------|--------------------------------|
-      # | v0.4.13                 | +:silence_deprecation_warning+ |
-      # | v0.5                    | +:warn+                        |
-      # | _eventually_            | +:raise+                       |
+      #   Note that calling IMAP#responses with a +type+ and without a block is
+      #   not configurable and always behaves like +:frozen_dup+.
+      #
+      #   <em>(+:frozen_dup+ config option was added in +v0.4.17+)</em>
+      #
+      # [+:raise+]
+      #   Raise an ArgumentError with the deprecation warning.
+      #
+      # Note: #responses_without_args is an alias for #responses_without_block.
       attr_accessor :responses_without_block, type: [
-        :silence_deprecation_warning, :warn, :raise,
+        :silence_deprecation_warning, :warn, :frozen_dup, :raise,
       ]
 
+      alias responses_without_args  responses_without_block  # :nodoc:
+      alias responses_without_args= responses_without_block= # :nodoc:
+
+      ##
+      # :attr_accessor: responses_without_args
+      #
+      # Alias for responses_without_block
+
       # Creates a new config object and initialize its attribute with +attrs+.
       #
       # If +parent+ is not given, the global config is used by default.
@@ -357,12 +388,11 @@ def defaults_hash
 
       version_defaults[0.5] = Config[:current]
 
-      version_defaults[0.6] = Config[0.5]
-      version_defaults[:next] = Config[0.6]
-
-      version_defaults[:future]  = Config[0.6].dup.update(
-        responses_without_block: :raise,
+      version_defaults[0.6] = Config[0.5].dup.update(
+        responses_without_block: :frozen_dup,
       ).freeze
+      version_defaults[:next] = Config[0.6]
+      version_defaults[:future] = Config[:next]
 
       version_defaults.freeze
     end
diff --git a/test/lib/helper.rb b/test/lib/helper.rb
@@ -2,3 +2,16 @@
 require "core_assertions"
 
 Test::Unit::TestCase.include Test::Unit::CoreAssertions
+
+class Test::Unit::TestCase
+  def wait_for_response_count(imap, type:, count:,
+                              timeout: 0.5, interval: 0.001)
+    deadline = Time.now + timeout
+    loop do
+      current_count = imap.responses(type, &:size)
+      break :count    if count <= current_count
+      break :deadline if deadline < Time.now
+      sleep interval
+    end
+  end
+end
diff --git a/test/net/imap/test_config.rb b/test/net/imap/test_config.rb
@@ -190,7 +190,7 @@ class ConfigTest < Test::Unit::TestCase
     assert_same Config.default, Config.new(Config.default).parent
     assert_same Config.global,  Config.new(Config.global).parent
     assert_same Config[0.4],    Config.new(0.4).parent
-    assert_same Config[0.5],    Config.new(:next).parent
+    assert_same Config[0.6],    Config.new(:next).parent
     assert_equal true, Config.new({debug: true}, debug: false).parent.debug?
     assert_equal true, Config.new({debug: true}, debug: false).parent.frozen?
   end
diff --git a/test/net/imap/test_imap.rb b/test/net/imap/test_imap.rb
diff --git a/test/net/imap/test_imap_responses.rb b/test/net/imap/test_imap_responses.rb
