Merge pull request #1066 from petergoldstein/fix/socket-compatibility-996-1012

petergoldstein · web-flow · commit ab534ffd16f8 · 2026-02-04T21:44:17.000-07:00
Release 4.3.1: Socket compatibility fixes and Shopify contributions
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,40 @@
 Dalli Changelog
 =====================
 
+4.3.1
+==========
+
+Bug Fixes:
+
+- Fix socket compatibility with gems that monkey-patch TCPSocket (#996, #1012)
+  - Gems like `socksify` and `resolv-replace` modify `TCPSocket#initialize`, breaking Ruby 3.0+'s `connect_timeout:` keyword argument
+  - Detection now uses parameter signature checking instead of gem-specific method detection
+  - Falls back to `Timeout.timeout` when monkey-patching is detected
+  - Detection result is cached for performance
+
+- Fix network retry bug with `socket_max_failures: 0` (#1065)
+  - Previously, setting `socket_max_failures: 0` could still cause retries due to error handling
+  - Introduced `RetryableNetworkError` subclass to distinguish retryable vs non-retryable errors
+  - `down!` now raises non-retryable `NetworkError`, `reconnect!` raises `RetryableNetworkError`
+  - Thanks to Graham Cooper (Shopify) for this fix
+
+- Fix "character class has duplicated range" Ruby warning (#1067)
+  - Fixed regex in `KeyManager::VALID_NAMESPACE_SEPARATORS` that caused warnings on newer Ruby versions
+  - Thanks to Hartley McGuire for this fix
+
+Improvements:
+
+- Add StrictWarnings test helper to catch Ruby warnings early (#1067)
+
+- Use bulk attribute setter for OpenTelemetry spans (#1068)
+  - Reduces lock acquisitions when setting span attributes
+  - Thanks to Robert Laurin (Shopify) for this optimization
+
+- Fix double recording of exceptions on OpenTelemetry spans (#1069)
+  - OpenTelemetry's `in_span` method already records exceptions and sets error status automatically
+  - Removed redundant explicit exception recording that caused exceptions to appear twice in traces
+  - Thanks to Robert Laurin (Shopify) for this fix
+
 4.3.0
 ==========
 
diff --git a/Gemfile b/Gemfile
@@ -27,4 +27,8 @@ end
 
 group :test do
   gem 'ruby-prof', platform: :mri
+
+  # For socket compatibility testing (these gems monkey-patch TCPSocket)
+  gem 'resolv-replace', require: false
+  gem 'socksify', require: false
 end
diff --git a/lib/dalli.rb b/lib/dalli.rb
@@ -28,6 +28,9 @@ class NotPermittedMultiOpError < DalliError; end
   # raised when Memcached response with a SERVER_ERROR
   class ServerError < DalliError; end
 
+  # socket/server communication error that can be retried
+  class RetryableNetworkError < NetworkError; end
+
   # Implements the NullObject pattern to store an application-defined value for 'Key not found' responses.
   class NilObject; end # rubocop:disable Lint/EmptyClass
   NOT_FOUND = NilObject.new
diff --git a/lib/dalli/client.rb b/lib/dalli/client.rb
@@ -522,8 +522,10 @@ def with
     def record_hit_miss_metrics(span, key_count, hit_count)
       return unless span
 
-      span.set_attribute('db.memcached.hit_count', hit_count)
-      span.set_attribute('db.memcached.miss_count', key_count - hit_count)
+      span.add_attributes({
+                            'db.memcached.hit_count' => hit_count,
+                            'db.memcached.miss_count' => key_count - hit_count
+                          })
     end
 
     def get_multi_yielding(keys)
@@ -622,7 +624,7 @@ def perform(*all_args)
                             }) do
         server.request(op, key, *args)
       end
-    rescue NetworkError => e
+    rescue RetryableNetworkError => e
       Dalli.logger.debug { e.inspect }
       Dalli.logger.debug { 'retrying request with new server' }
       retry
diff --git a/lib/dalli/instrumentation.rb b/lib/dalli/instrumentation.rb
@@ -90,12 +90,8 @@ def enabled?
       def trace(name, attributes = {})
         return yield unless enabled?
 
-        tracer.in_span(name, attributes: DEFAULT_ATTRIBUTES.merge(attributes), kind: :client) do |span|
+        tracer.in_span(name, attributes: DEFAULT_ATTRIBUTES.merge(attributes), kind: :client) do |_span|
           yield
-        rescue StandardError => e
-          span.record_exception(e)
-          span.status = OpenTelemetry::Trace::Status.error(e.message)
-          raise
         end
       end
 
@@ -123,16 +119,10 @@ def trace(name, attributes = {})
       #     results
       #   end
       #
-      def trace_with_result(name, attributes = {})
+      def trace_with_result(name, attributes = {}, &)
         return yield(nil) unless enabled?
 
-        tracer.in_span(name, attributes: DEFAULT_ATTRIBUTES.merge(attributes), kind: :client) do |span|
-          yield(span)
-        rescue StandardError => e
-          span.record_exception(e)
-          span.status = OpenTelemetry::Trace::Status.error(e.message)
-          raise
-        end
+        tracer.in_span(name, attributes: DEFAULT_ATTRIBUTES.merge(attributes), kind: :client, &)
       end
     end
   end
diff --git a/lib/dalli/key_manager.rb b/lib/dalli/key_manager.rb
@@ -31,7 +31,7 @@ class KeyManager
 
     # Valid separators: non-alphanumeric, single printable ASCII characters
     # Excludes: alphanumerics, whitespace, control characters
-    VALID_NAMESPACE_SEPARATORS = /\A[^a-zA-Z0-9\s\x00-\x1F\x7F]\z/
+    VALID_NAMESPACE_SEPARATORS = /\A[^a-zA-Z0-9 \x00-\x1F\x7F]\z/
 
     def initialize(client_options)
       @key_options =
diff --git a/lib/dalli/pipelined_getter.rb b/lib/dalli/pipelined_getter.rb
@@ -36,7 +36,7 @@ def process(keys, &block)
 
         servers = fetch_responses(servers, start_time, @ring.socket_timeout, &block) until servers.empty?
       end
-    rescue NetworkError => e
+    rescue Dalli::RetryableNetworkError => e
       Dalli.logger.debug { e.inspect }
       Dalli.logger.debug { 'retrying pipelined gets because of timeout' }
       retry
@@ -143,7 +143,7 @@ def fetch_responses(servers, start_time, timeout, &block)
       servers
     rescue NetworkError
       # Abort and raise if we encountered a network error.  This triggers
-      # a retry at the top level.
+      # a retry at the top level on RetryableNetworkError.
       abort_without_timeout(servers)
       raise
     end
diff --git a/lib/dalli/pipelined_setter.rb b/lib/dalli/pipelined_setter.rb
@@ -28,7 +28,7 @@ def process(hash, ttl, req_options)
         servers = setup_requests(hash, ttl, req_options)
         finish_requests(servers)
       end
-    rescue NetworkError => e
+    rescue Dalli::RetryableNetworkError => e
       Dalli.logger.debug { e.inspect }
       Dalli.logger.debug { 'retrying pipelined sets because of network error' }
       retry
diff --git a/lib/dalli/protocol/connection_manager.rb b/lib/dalli/protocol/connection_manager.rb
@@ -199,7 +199,7 @@ def error_on_request!(err_or_string)
       def reconnect!(message)
         close
         sleep(options[:socket_failure_delay]) if options[:socket_failure_delay]
-        raise Dalli::NetworkError, message
+        raise Dalli::RetryableNetworkError, message
       end
 
       def reset_down_info
diff --git a/lib/dalli/socket.rb b/lib/dalli/socket.rb
@@ -90,6 +90,12 @@ class TCP < TCPSocket
       # options - supports enhanced logging in the case of a timeout
       attr_accessor :options
 
+      # Expected parameter signature for unmodified TCPSocket#initialize.
+      # Used to detect when gems like socksify or resolv-replace have monkey-patched
+      # TCPSocket, which breaks the connect_timeout: keyword argument.
+      TCPSOCKET_NATIVE_PARAMETERS = [[:rest]].freeze
+      private_constant :TCPSOCKET_NATIVE_PARAMETERS
+
       def self.open(host, port, options = {})
         create_socket_with_timeout(host, port, options) do |sock|
           sock.options = { host: host, port: port }.merge(options)
@@ -99,15 +105,18 @@ def self.open(host, port, options = {})
         end
       end
 
+      # Detect and cache whether TCPSocket supports the connect_timeout: keyword argument.
+      # Returns false if TCPSocket#initialize has been monkey-patched by gems like
+      # socksify or resolv-replace, which don't support keyword arguments.
+      def self.supports_connect_timeout?
+        return @supports_connect_timeout if defined?(@supports_connect_timeout)
+
+        @supports_connect_timeout = RUBY_VERSION >= '3.0' &&
+                                    ::TCPSocket.instance_method(:initialize).parameters == TCPSOCKET_NATIVE_PARAMETERS
+      end
+
       def self.create_socket_with_timeout(host, port, options)
-        # Check that TCPSocket#initialize was not overwritten by resolv-replace gem
-        # (part of ruby standard library since 3.0.0, should be removed in 3.4.0),
-        # as it does not handle keyword arguments correctly.
-        # To check this we are using the fact that resolv-replace
-        # aliases TCPSocket#initialize method to #original_resolv_initialize.
-        # https://github.com/ruby/resolv-replace/blob/v0.1.1/lib/resolv-replace.rb#L21
-        if RUBY_VERSION >= '3.0' &&
-           !::TCPSocket.private_method_defined?(:original_resolv_initialize)
+        if supports_connect_timeout?
           sock = new(host, port, connect_timeout: options[:socket_timeout])
           yield(sock)
         else
diff --git a/lib/dalli/version.rb b/lib/dalli/version.rb
@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 module Dalli
-  VERSION = '4.3.0'
+  VERSION = '4.3.1'
 
   MIN_SUPPORTED_MEMCACHED_VERSION = '1.4'
 end
diff --git a/test/helper.rb b/test/helper.rb
@@ -1,5 +1,16 @@
 # frozen_string_literal: true
 
+module StrictWarnings
+  def warn(msg, **, &)
+    # Allow intentional deprecation warnings from Dalli
+    return super if msg.to_s.start_with?('[DEPRECATION]')
+
+    raise RuntimeError, msg, caller(1)
+  end
+end
+
+Warning.singleton_class.prepend(StrictWarnings)
+
 require 'bundler/setup'
 # require 'simplecov'
 # SimpleCov.start
diff --git a/test/integration/test_network.rb b/test/integration/test_network.rb
@@ -44,7 +44,7 @@
 
           it 'handle socket timeouts' do
             dc = Dalli::Client.new('localhost:19123', socket_timeout: 0)
-            assert_raises Dalli::RingError, message: 'No server available' do
+            assert_raises Dalli::RingError do
               dc.get('abc')
             end
           end
@@ -86,12 +86,37 @@
           }) do
             dc = Dalli::Client.new('localhost:19123', socket_timeout: 0.1, protocol: p, socket_max_failures: 0,
                                                       socket_failure_delay: 0.0, down_retry_delay: 0.0)
-            assert_raises Dalli::RingError, message: 'No server available' do
+            # With socket_max_failures: 0, the first error triggers down! which raises NetworkError.
+            # This NetworkError is not retried (only RetryableNetworkError is).
+            assert_raises Dalli::NetworkError do
               dc.get('abc')
             end
           end
         end
 
+        it 'does not retry NetworkError from down! with socket_max_failures: 0' do
+          next if p == :binary
+
+          memcached_mock(lambda { |sock|
+            # handle initial version call
+            sock.gets
+            sock.write("VERSION 1.6.0\r\n")
+
+            # handle the get request but never respond, causing timeout
+            sock.gets
+            sleep(0.3)
+          }) do
+            dc = Dalli::Client.new('localhost:19123', socket_timeout: 0.1, protocol: p, socket_max_failures: 0,
+                                                      socket_failure_delay: 0.0, down_retry_delay: 0.0)
+            # With socket_max_failures: 0, the first error triggers down! which raises NetworkError.
+            # This NetworkError should NOT be caught and retried by perform (only RetryableNetworkError is).
+            err = assert_raises Dalli::NetworkError do
+              dc.get('abc')
+            end
+            assert_match(/is down/, err.message)
+          end
+        end
+
         it 'opens a standard TCP connection when ssl_context is not configured' do
           memcached_persistent(p) do |dc|
             server = dc.send(:ring).servers.first
@@ -179,6 +204,9 @@
       end
 
       it 'handles SSL error during read operations' do
+        # JRuby's socket classes don't support Minitest's stub method aliasing
+        skip 'Minitest stub incompatible with JRuby sockets' if RUBY_ENGINE == 'jruby'
+
         with_nil_logger do
           memcached(p, 19_191) do |dc|
             # First set a value so we have something to get
@@ -229,6 +257,10 @@
         with_nil_logger do
           memcached(p, 19_191) do |dc|
             10.times do |i|
+              # Pre-set the key so we're testing Thread#raise handling,
+              # not whether the set completed before interruption
+              dc.set("key:#{i}", i.to_s)
+
               thread = Thread.new do
                 loop do
                   assert_instance_of Integer, dc.set("key:#{i}", i.to_s)
@@ -243,6 +275,7 @@
 
               refute_nil joined_thread
               refute_predicate joined_thread, :alive?
+              # Verify the connection is still usable after Thread#raise
               assert_equal i.to_s, dc.get("key:#{i}")
             end
           end
diff --git a/test/test_gem_compatibility.rb b/test/test_gem_compatibility.rb
diff --git a/test/test_instrumentation.rb b/test/test_instrumentation.rb
diff --git a/test/test_socket.rb b/test/test_socket.rb
