Prepare 5.0.8 release

CHANGELOG.md

@@ -4,6 +4,22 @@ Dalli Changelog
 Unreleased
 ==========
 
+5.0.8
+==========
+
+Bug fixes:
+
+- Fix multi-server `get_multi` stopping at an empty value (#1170)
+  - The pipelined reply parser took a hit on a zero-length value (`VA 0`) for the terminating `MN`, so that server's remaining keys were silently missing from the result
+  - Its value terminator and any replies not yet parsed stayed on the connection and were read as the replies to later commands on it. Those commands could fail with `Dalli::DalliError: Response error`, or a single-key `get` could silently return another key's value
+  - An empty value is now returned as `''`, like a single-key `get` and single-server `get_multi`
+  - Affects the meta protocol since 3.2.0 (the default since 5.0.0)
+  - Thanks to Julian Richard Contreras for this contribution
+
+Development:
+
+- Fix offenses reported by RuboCop 1.91 and require `rubocop >= 1.91` (backport of #1162)
+
 5.0.7
 ==========
 

Gemfile

@@ -18,7 +18,7 @@ group :development, :test do
   gem 'rack', '~> 3'
   gem 'rack-session'
   gem 'rake', '~> 13.0'
-  gem 'rubocop'
+  gem 'rubocop', '>= 1.91' # disable-next directives require 1.91+
   gem 'rubocop-minitest'
   gem 'rubocop-performance'
   gem 'rubocop-rake'

lib/dalli/client.rb

@@ -2,7 +2,6 @@
 
 require 'digest/md5'
 
-# encoding: ascii
 module Dalli
   ##
   # Dalli::Client is the main class which developers will use to interact with
@@ -144,7 +143,7 @@ def get_with_metadata(key, options = {})
     # Fetch multiple keys efficiently.
     # If a block is given, yields key/value pairs one at a time.
     # Otherwise returns a hash of { 'key' => 'value', 'key2' => 'value1' }
-    # rubocop:disable Style/ExplicitBlockArgument
+    # rubocop:disable-next Style/ExplicitBlockArgument
     def get_multi(*keys)
       keys.flatten!
       keys.compact!
@@ -156,7 +155,6 @@ def get_multi(*keys)
         get_multi_hash(keys)
       end
     end
-    # rubocop:enable Style/ExplicitBlockArgument
 
     ##
     # Fetch multiple keys efficiently, including available metadata such as CAS.

lib/dalli/protocol/meta.rb

@@ -153,15 +153,14 @@ def prepend(key, value)
         response_processor.meta_set_append_prepend unless quiet?
       end
 
-      # rubocop:disable Metrics/ParameterLists
+      # rubocop:disable-next Metrics/ParameterLists
       def write_append_prepend_req(mode, key, value, ttl = nil, cas = nil, _options = {})
         ttl = TtlSanitizer.sanitize(ttl) if ttl
         req = RequestFormatter.meta_set(key: key, value: value,
                                         cas: cas, ttl: ttl, mode: mode, quiet: quiet?)
         write("#{req}#{value}#{TERMINATOR}")
         @connection_manager.flush unless quiet?
       end
-      # rubocop:enable Metrics/ParameterLists
 
       # Delete Commands
       def delete(key, cas)

lib/dalli/protocol/response_processor.rb

@@ -204,8 +204,10 @@ def getk_response_from_buffer(buf, offset = 0)
           # We have a complete response that has no body.
           # This is either the response to the terminating
           # noop or, if the status is not MN, an intermediate
-          # error response that needs to be discarded.
-          return [true, header_len] if body_len.zero?
+          # error response that needs to be discarded. A hit
+          # on an empty value (VA 0) still has a body -- just
+          # its terminator -- so it's parsed below as a value.
+          return [true, header_len] if body_len.zero? && tokens.first != VA
 
           resp_size = header_len + body_len + TERMINATOR.length
           # The header is in the buffer, but the body is not.  As we don't have

lib/dalli/socket.rb

@@ -124,7 +124,7 @@ def self.open(host, port, options = {})
       # Returns true for an unmodified TCPSocket on Ruby 3.0+, or for resolv-replace >= 0.2.0
       # which forwards keyword arguments through its patch.
       # Returns false when monkey-patched by gems like socksify or resolv-replace < 0.2.0.
-      # rubocop:disable ThreadSafety/ClassInstanceVariable
+      # rubocop:disable-next ThreadSafety/ClassInstanceVariable
       def self.supports_connect_timeout?
         return @supports_connect_timeout if defined?(@supports_connect_timeout)
 
@@ -135,7 +135,6 @@ def self.supports_connect_timeout?
                                       end
                                     end
       end
-      # rubocop:enable ThreadSafety/ClassInstanceVariable
 
       def self.create_socket_with_timeout(host, port, options)
         if supports_connect_timeout?
@@ -192,14 +191,13 @@ def self.configure_timeout(sock, options)
 
       # Detect and cache the correct pack format for struct timeval on this platform.
       # Different architectures have different sizes for time_t and suseconds_t.
-      # rubocop:disable ThreadSafety/ClassInstanceVariable
+      # rubocop:disable-next ThreadSafety/ClassInstanceVariable
       def self.timeval_pack_format(sock)
         @timeval_pack_format ||= begin
           expected_size = sock.getsockopt(::Socket::SOL_SOCKET, ::Socket::SO_RCVTIMEO).data.bytesize
           TIMEVAL_PACK_FORMATS.find { |fmt| TIMEVAL_TEST_VALUES.pack(fmt).bytesize == expected_size } || 'll'
         end
       end
-      # rubocop:enable ThreadSafety/ClassInstanceVariable
 
       def self.pack_timeval(sock, seconds, microseconds)
         [seconds, microseconds].pack(timeval_pack_format(sock))

lib/dalli/version.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 module Dalli
-  VERSION = '5.0.7'
+  VERSION = '5.0.8'
 
   MIN_SUPPORTED_MEMCACHED_VERSION = '1.6'
 end

lib/rack/session/dalli.rb

@@ -16,11 +16,10 @@ class MissingSessionError < StandardError; end
       attr_reader :data
 
       # Don't freeze this until we fix the specs/implementation
-      # rubocop:disable Style/MutableConstant
+      # rubocop:disable-next Style/MutableConstant
       DEFAULT_DALLI_OPTIONS = {
         namespace: 'rack:session'
       }
-      # rubocop:enable Style/MutableConstant
 
       # Brings in a new Rack::Session::Dalli middleware with the given
       # `:memcache_server`. The server is either a hostname, or a

scripts/install_memcached.sh

@@ -5,7 +5,10 @@ set -euo pipefail
 version=$MEMCACHED_VERSION
 
 sudo apt-get -y remove memcached
-sudo apt-get install libevent-dev
+# Refresh the runner image's package index first: when Ubuntu replaces a package
+# version, the stale index points at files the mirrors no longer serve (404).
+sudo apt-get update
+sudo apt-get -y install libevent-dev
 
 echo Installing Memcached version ${version}
 

test/benchmark_test.rb

@@ -113,7 +113,7 @@ def profile(&)
           end
 
           @m = Dalli::Client.new(@servers, protocol: protocol)
-          # rubocop:disable Lint/SuppressedException
+          # rubocop:disable-next Lint/SuppressedException
           x.report('missing:ruby:dalli') do
             n.times do
               begin @m.delete @key1; rescue StandardError; end
@@ -124,7 +124,6 @@ def profile(&)
               begin @m.get @key3; rescue StandardError; end
             end
           end
-          # rubocop:enable Lint/SuppressedException
 
           @m = Dalli::Client.new(@servers, protocol: protocol)
           x.report('mixed:ruby:dalli') do

test/integration/test_fiber_concurrency.rb

@@ -6,9 +6,8 @@
 # StandardError) so that blanket `rescue StandardError` clauses don't
 # accidentally swallow scheduler-driven fiber cancellation.  We reproduce that
 # hierarchy locally to avoid pulling the `async` gem just for the signal class.
-# rubocop:disable Lint/InheritException
+# rubocop:disable-next Lint/InheritException
 class FiberCancellation < Exception; end
-# rubocop:enable Lint/InheritException
 
 describe 'fiber concurrency' do
   # `Async::Stop < Exception` but NOT `< StandardError`, so when the scheduler

test/integration/test_pipelined_get.rb

@@ -237,3 +237,32 @@
     end
   end
 end
+
+describe 'Pipelined Get on a multi-server ring' do
+  MemcachedManager.supported_protocols.each do |p|
+    describe "using the #{p} protocol" do
+      it 'returns an empty value without cutting off the rest of its server' do
+        memcached_persistent(p, 21_345) do |_, port1|
+          memcached_persistent(p, 21_346) do |_, port2|
+            dc = Dalli::Client.new(["localhost:#{port1}", "localhost:#{port2}"], raw: true)
+            dc.flush
+            big = 'x' * 20_000
+            many = Array.new(100) { |i| "key#{i}" }
+            many.each { |k| dc.set(k, big) }
+            dc.set('key5', '')
+            expected = many.to_h { |k| [k, k == 'key5' ? '' : big] }
+
+            assert_equal expected, dc.get_multi(many)
+
+            yielded = {}
+            dc.get_multi(many) { |k, v| yielded[k] = v }
+
+            assert_equal expected, yielded
+            # Nothing is left unread on either connection
+            many.each { |k| assert_equal expected[k], dc.get(k) }
+          end
+        end
+      end
+    end
+  end
+end

test/protocol/test_response_processor.rb

@@ -364,5 +364,21 @@ def expect_read_data(data, size)
       assert_equal 4, result.last # header length
       assert result[0] # ok status
     end
+
+    it 'returns an empty value for a VA 0 hit, consuming its terminator' do
+      buf = "VA 0 f0 kfoo s0\r\n\r\nMN\r\n".b
+
+      status, cas, key, value, size = processor.getk_response_from_buffer(buf)
+
+      assert status
+      assert_equal 0, cas
+      assert_equal 'foo', key
+      assert_equal '', value
+      assert_equal "VA 0 f0 kfoo s0\r\n\r\n".bytesize, size
+    end
+
+    it 'returns [0] for a VA 0 hit whose terminator has not arrived' do
+      assert_equal [0], processor.getk_response_from_buffer("VA 0 f0 kfoo s0\r\n".b)
+    end
   end
 end

test/protocol/test_value_serializer.rb

@@ -6,9 +6,8 @@
   describe 'marshal security warning' do
     before do
       # Reset the class variable before each test
-      # rubocop:disable Style/ClassVars
+      # rubocop:disable-next Style/ClassVars
       Dalli::Protocol::ValueSerializer.class_variable_set(:@@marshal_warning_logged, false)
-      # rubocop:enable Style/ClassVars
     end
 
     it 'logs a warning when using default Marshal serializer' do