RUBY-1986 Retry reads in legacy mode on OperationFailure errors that modern mode retries on (#1562)

Author: p-mongo

lib/mongo/error/operation_failure.rb

@@ -80,15 +80,15 @@ class OperationFailure < Error
       # @since 2.6.0
       attr_reader :code_name
 
-      # Whether the error is a retryable error according to the legacy read retry
-      # logic.
+      # Whether the error is a retryable error according to the legacy
+      # read retry logic.
       #
       # @return [ true, false ]
       #
       # @since 2.1.1
       # @deprecated
       def retryable?
-        RETRY_MESSAGES.any?{ |m| message.include?(m) }
+        write_retryable? || RETRY_MESSAGES.any?{ |m| message.include?(m) }
       end
 
       # Whether the error is a retryable error according to the modern retryable

lib/mongo/retryable.rb

@@ -333,7 +333,7 @@ def legacy_read_with_retry(session, server_selector)
         retry
       rescue Error::OperationFailure => e
         e.add_note("attempt #{attempt + 1}")
-        if cluster.sharded? && e.retryable? && !(session && session.in_transaction?)
+        if e.retryable? && !(session && session.in_transaction?)
           if attempt > client.max_read_retries
             raise e
           end

spec/mongo/retryable_spec.rb

@@ -175,83 +175,62 @@ class RetryableHost
 
     context 'when an operation failure occurs' do
 
-      context 'when the cluster is not a mongos' do
+      context 'when the operation failure is not retryable' do
+
+        let(:error) do
+          Mongo::Error::OperationFailure.new('not authorized')
+        end
 
         before do
-          expect(operation).to receive(:execute).and_raise(Mongo::Error::OperationFailure).ordered
-          expect(cluster).to receive(:sharded?).and_return(false)
+          expect(operation).to receive(:execute).and_raise(error).ordered
         end
 
-        it 'raises an exception' do
+        it 'raises the exception' do
           expect {
             read_operation
           }.to raise_error(Mongo::Error::OperationFailure)
         end
       end
 
-      context 'when the cluster is a mongos' do
+      context 'when the operation failure is retryable' do
 
-        context 'when the operation failure is not retryable' do
+        let(:error) do
+          Mongo::Error::OperationFailure.new('not master')
+        end
 
-          let(:error) do
-            Mongo::Error::OperationFailure.new('not authorized')
-          end
+        context 'when the retry succeeds' do
 
           before do
+            expect(retryable).to receive(:select_server).ordered
             expect(operation).to receive(:execute).and_raise(error).ordered
-            expect(cluster).to receive(:sharded?).and_return(true)
+            expect(client).to receive(:read_retry_interval).and_return(0.1).ordered
+            expect(retryable).to receive(:select_server).ordered
+            expect(operation).to receive(:execute).and_return(true).ordered
           end
 
-          it 'raises the exception' do
-            expect {
-              read_operation
-            }.to raise_error(Mongo::Error::OperationFailure)
+          it 'returns the result' do
+            expect(read_operation).to be true
           end
         end
 
-        context 'when the operation failure is retryable' do
+        context 'when the retry fails once and then succeeds' do
+          let(:max_read_retries) { 2 }
 
-          let(:error) do
-            Mongo::Error::OperationFailure.new('not master')
-          end
-
-          context 'when the retry succeeds' do
+          before do
+            expect(retryable).to receive(:select_server).ordered
+            expect(operation).to receive(:execute).and_raise(error).ordered
 
-            before do
-              expect(retryable).to receive(:select_server).ordered
-              expect(operation).to receive(:execute).and_raise(error).ordered
-              expect(cluster).to receive(:sharded?).and_return(true)
-              expect(client).to receive(:read_retry_interval).and_return(0.1).ordered
-              expect(retryable).to receive(:select_server).ordered
-              expect(operation).to receive(:execute).and_return(true).ordered
-            end
+            expect(client).to receive(:read_retry_interval).and_return(0.1).ordered
+            expect(retryable).to receive(:select_server).ordered
+            expect(operation).to receive(:execute).and_raise(error).ordered
 
-            it 'returns the result' do
-              expect(read_operation).to be true
-            end
+            expect(client).to receive(:read_retry_interval).and_return(0.1).ordered
+            expect(retryable).to receive(:select_server).ordered
+            expect(operation).to receive(:execute).and_return(true).ordered
           end
 
-          context 'when the retry fails once and then succeeds' do
-            let(:max_read_retries) { 2 }
-
-            before do
-              expect(retryable).to receive(:select_server).ordered
-              expect(operation).to receive(:execute).and_raise(error).ordered
-
-              expect(cluster).to receive(:sharded?).and_return(true)
-              expect(client).to receive(:read_retry_interval).and_return(0.1).ordered
-              expect(retryable).to receive(:select_server).ordered
-              expect(operation).to receive(:execute).and_raise(error).ordered
-
-              expect(cluster).to receive(:sharded?).and_return(true)
-              expect(client).to receive(:read_retry_interval).and_return(0.1).ordered
-              expect(retryable).to receive(:select_server).ordered
-              expect(operation).to receive(:execute).and_return(true).ordered
-            end
-
-            it 'returns the result' do
-              expect(read_operation).to be true
-            end
+          it 'returns the result' do
+            expect(read_operation).to be true
           end
         end
       end

spec/spec_tests/retryable_reads_spec.rb

@@ -12,3 +12,34 @@
     end
   end
 end
+
+describe 'Retryable reads spec tests - legacy' do
+  require_no_multi_shard
+
+  define_crud_spec_tests(RETRYABLE_READS_TESTS) do |spec, req, test|
+    let(:client_options) do
+      {
+        max_read_retries: 1,
+        read_retry_interval: 0,
+        retry_reads: false,
+      }.update(test.client_options)
+    end
+
+    let(:client) do
+      authorized_client.with(client_options).tap do |client|
+        client.subscribe(Mongo::Monitoring::COMMAND, event_subscriber)
+      end
+    end
+
+    around do |example|
+      desc = example.full_description
+      # Skip tests that disable modern retryable reads because they expect
+      # no retries - and since legacy retryable reads are used, the tests
+      # will fail.
+      if desc =~/retryReads is false|fails on first attempt/
+        skip 'Test not applicable to legacy read retries'
+      end
+      example.run
+    end
+  end
+end