diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b5927ea..28162875 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,22 @@ Dalli Changelog Unreleased ========== +4.3.5 +========== + +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 only clients using `protocol: :meta`; the default binary protocol is not affected + - Thanks to Julian Richard Contreras for this contribution + +Development: + +- Fix offenses reported by RuboCop 1.91 and require `rubocop >= 1.91` (backport of #1162) + 4.3.4 ========== diff --git a/Gemfile b/Gemfile index 9cd452d1..afa0f9a3 100644 --- a/Gemfile +++ b/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' diff --git a/bin/benchmark b/bin/benchmark index 143813c2..7a7c0e2c 100755 --- a/bin/benchmark +++ b/bin/benchmark @@ -20,6 +20,7 @@ require 'benchmark/ips' require 'monitor' require_relative '../lib/dalli' +# rubocop:disable Style/OneClassPerFile ## # NoopSerializer is a serializer that avoids the overhead of Marshal or JSON. ## @@ -273,3 +274,4 @@ if %w[all set_multi].include?(bench_target) x.compare! end end +# rubocop:enable Style/OneClassPerFile diff --git a/lib/dalli/client.rb b/lib/dalli/client.rb index 4f1ac61b..12838fab 100644 --- a/lib/dalli/client.rb +++ b/lib/dalli/client.rb @@ -3,7 +3,6 @@ require 'digest/md5' require 'set' -# encoding: ascii module Dalli ## # Dalli::Client is the main class which developers will use to interact with @@ -152,7 +151,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! @@ -164,7 +163,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. diff --git a/lib/dalli/protocol/base.rb b/lib/dalli/protocol/base.rb index 25f5f192..93de5923 100644 --- a/lib/dalli/protocol/base.rb +++ b/lib/dalli/protocol/base.rb @@ -94,7 +94,7 @@ def pipeline_response_setup # When a block is given, yields (key, value, cas) for each response, # avoiding intermediate Hash allocation. Returns nil. # Without a block, returns a Hash of { key => [value, cas] }. - # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity + # rubocop:disable-next Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity def pipeline_next_responses(&block) reconnect_on_pipeline_complete! values = nil @@ -128,7 +128,6 @@ def pipeline_next_responses(&block) rescue SystemCallError, *TIMEOUT_ERRORS, *SSL_ERRORS, EOFError => e @connection_manager.error_on_request!(e) end - # rubocop:enable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/PerceivedComplexity # Abort current pipelined get. Generally used to signal an external # timeout during pipelined get. The underlying socket is diff --git a/lib/dalli/protocol/binary.rb b/lib/dalli/protocol/binary.rb index 36299fed..686ed9fa 100644 --- a/lib/dalli/protocol/binary.rb +++ b/lib/dalli/protocol/binary.rb @@ -76,7 +76,7 @@ def replace(key, value, ttl, cas, options) storage_req(opkey, key, value, ttl, cas, options) end - # rubocop:disable Metrics/ParameterLists + # rubocop:disable-next Metrics/ParameterLists def storage_req(opkey, key, value, ttl, cas, options) (value, bitflags) = @value_marshaller.store(key, value, options) ttl = TtlSanitizer.sanitize(ttl) @@ -88,7 +88,6 @@ def storage_req(opkey, key, value, ttl, cas, options) @connection_manager.flush unless quiet? response_processor.storage_response unless quiet? end - # rubocop:enable Metrics/ParameterLists def append(key, value) opkey = quiet? ? :appendq : :append diff --git a/lib/dalli/protocol/binary/request_formatter.rb b/lib/dalli/protocol/binary/request_formatter.rb index a147f895..47fc683e 100644 --- a/lib/dalli/protocol/binary/request_formatter.rb +++ b/lib/dalli/protocol/binary/request_formatter.rb @@ -88,7 +88,7 @@ class RequestFormatter }.freeze FORMAT = BODY_FORMATS.transform_values { |v| REQ_HEADER_FORMAT + v } - # rubocop:disable Metrics/ParameterLists + # rubocop:disable-next Metrics/ParameterLists def self.standard_request(opkey:, key: nil, value: nil, opaque: 0, cas: 0, bitflags: nil, ttl: nil) extra_len = (bitflags.nil? ? 0 : 4) + (ttl.nil? ? 0 : 4) key_len = key.nil? ? 0 : key.bytesize @@ -97,7 +97,6 @@ def self.standard_request(opkey:, key: nil, value: nil, opaque: 0, cas: 0, bitfl body = [bitflags, ttl, key, value].compact (header + body).pack(FORMAT[opkey]) end - # rubocop:enable Metrics/ParameterLists def self.decr_incr_request(opkey:, key: nil, count: nil, initial: nil, expiry: nil) extra_len = 20 diff --git a/lib/dalli/protocol/meta.rb b/lib/dalli/protocol/meta.rb index 6fd4f018..8e793600 100644 --- a/lib/dalli/protocol/meta.rb +++ b/lib/dalli/protocol/meta.rb @@ -146,7 +146,7 @@ def replace(key, value, ttl, cas, options) response_processor.meta_set_with_cas unless quiet? end - # rubocop:disable Metrics/ParameterLists + # rubocop:disable-next Metrics/ParameterLists def write_storage_req(mode, key, raw_value, ttl = nil, cas = nil, options = {}, quiet: quiet?) (value, bitflags) = @value_marshaller.store(key, raw_value, options) ttl = TtlSanitizer.sanitize(ttl) if ttl @@ -159,7 +159,6 @@ def write_storage_req(mode, key, raw_value, ttl = nil, cas = nil, options = {}, write(TERMINATOR) @connection_manager.flush unless quiet end - # rubocop:enable Metrics/ParameterLists def append(key, value) write_append_prepend_req(:append, key, value) @@ -171,7 +170,7 @@ 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 encoded_key, base64 = KeyRegularizer.encode(key) @@ -182,7 +181,6 @@ def write_append_prepend_req(mode, key, value, ttl = nil, cas = nil, _options = write(TERMINATOR) @connection_manager.flush unless quiet? end - # rubocop:enable Metrics/ParameterLists # Delete Commands def delete(key, cas) diff --git a/lib/dalli/protocol/meta/response_processor.rb b/lib/dalli/protocol/meta/response_processor.rb index 32bbe86b..23854e7f 100644 --- a/lib/dalli/protocol/meta/response_processor.rb +++ b/lib/dalli/protocol/meta/response_processor.rb @@ -175,8 +175,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 [header_len, true, nil, nil, nil] 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 [header_len, true, nil, nil, nil] if no_body?(tokens, body_len) resp_size = header_len + body_len + TERMINATOR.length # The header is in the buffer, but the body is not. As we don't have @@ -189,6 +191,11 @@ def getk_response_from_buffer(buf, offset = 0) full_response_from_buffer(tokens, body, resp_size) end + # A zero-size reply has no body unless it's a VA (a hit on an empty value) + def no_body?(tokens, body_len) + body_len.zero? && tokens.first != VA + end + def error_on_unexpected!(expected_codes) tokens = next_line_to_tokens diff --git a/lib/dalli/version.rb b/lib/dalli/version.rb index 892781e6..2294e746 100644 --- a/lib/dalli/version.rb +++ b/lib/dalli/version.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module Dalli - VERSION = '4.3.4' + VERSION = '4.3.5' MIN_SUPPORTED_MEMCACHED_VERSION = '1.4' end diff --git a/lib/rack/session/dalli.rb b/lib/rack/session/dalli.rb index 8e9fc4b8..6b22187b 100644 --- a/lib/rack/session/dalli.rb +++ b/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 diff --git a/scripts/install_memcached.sh b/scripts/install_memcached.sh index 8b896e45..cb3e18bc 100644 --- a/scripts/install_memcached.sh +++ b/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 libsasl2-dev sasl2-bin +# 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 libsasl2-dev sasl2-bin echo Installing Memcached version ${version} diff --git a/test/benchmark_test.rb b/test/benchmark_test.rb index 4b4de941..393e0c29 100644 --- a/test/benchmark_test.rb +++ b/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 diff --git a/test/helpers/memcached.rb b/test/helpers/memcached.rb index 83f812b9..938b2494 100644 --- a/test/helpers/memcached.rb +++ b/test/helpers/memcached.rb @@ -45,11 +45,10 @@ def memcached(protocol, port_or_socket, args = '', client_options = {}, terminat # Launches a memcached process using the memcached method in this module, # but sets terminate_process to false ensuring that the process persists # past execution of the block argument. - # rubocop:disable Metrics/ParameterLists + # rubocop:disable-next Metrics/ParameterLists def memcached_persistent(protocol = :binary, port_or_socket = 21_345, args = '', client_options = {}, &) memcached(protocol, port_or_socket, args, client_options, terminate_process: false, &) end - # rubocop:enable Metrics/ParameterLists # Launches a persistent memcached process, configured to use SSL def memcached_ssl_persistent(protocol = :binary, port_or_socket = rand(21_397..21_896), &) diff --git a/test/integration/test_pipelined_get.rb b/test/integration/test_pipelined_get.rb index af2e8fa5..d5009872 100644 --- a/test/integration/test_pipelined_get.rb +++ b/test/integration/test_pipelined_get.rb @@ -142,3 +142,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, protocol: p) + 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 diff --git a/test/protocol/binary/test_response_processor.rb b/test/protocol/binary/test_response_processor.rb index 9b089a7b..39ded29e 100644 --- a/test/protocol/binary/test_response_processor.rb +++ b/test/protocol/binary/test_response_processor.rb @@ -7,7 +7,7 @@ # Format: magic(1) + opcode(1) + key_len(2) + extra_len(1) + data_type(1) + # status(2) + body_len(4) + opaque(4) + cas(8) # Note: CAS uses native endian (Q) to match ResponseHeader's FMT = '@2nCCnNNQ' - # rubocop:disable Metrics/ParameterLists + # rubocop:disable-next Metrics/ParameterLists def create_header(status: 0, key_len: 0, extra_len: 0, body_len: 0, cas: 0, opaque: 0) [ 0x81, # magic (response) @@ -21,7 +21,6 @@ def create_header(status: 0, key_len: 0, extra_len: 0, body_len: 0, cas: 0, opaq cas # CAS (native endian) ].pack('CCnCCnNNQ') end - # rubocop:enable Metrics/ParameterLists let(:io_source) { Minitest::Mock.new } let(:value_marshaller) { Dalli::Protocol::ValueMarshaller.new({}) } diff --git a/test/protocol/meta/test_response_processor.rb b/test/protocol/meta/test_response_processor.rb index 1e3dc705..7703f18f 100644 --- a/test/protocol/meta/test_response_processor.rb +++ b/test/protocol/meta/test_response_processor.rb @@ -337,5 +337,21 @@ def expect_read_data(data, size) assert_equal 4, result[0] # header length assert result[1] # 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 + + size, status, cas, key, value = processor.getk_response_from_buffer(buf) + + assert_equal "VA 0 f0 kfoo s0\r\n\r\n".bytesize, size + assert status + assert_equal 0, cas + assert_equal 'foo', key + assert_equal '', value + end + + it 'waits for the terminator of a VA 0 hit' do + assert_equal [0, nil, nil, nil, nil], processor.getk_response_from_buffer("VA 0 f0 kfoo s0\r\n".b) + end end end diff --git a/test/protocol/test_value_serializer.rb b/test/protocol/test_value_serializer.rb index e182f58b..e3a03547 100644 --- a/test/protocol/test_value_serializer.rb +++ b/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