Skip to content

RUBY-2682 RUBY-2443 test that driver closes cursors left open by server when result set is fully iterated #2378

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Dec 14, 2021
Merged
10 changes: 9 additions & 1 deletion lib/mongo/cursor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -383,6 +383,14 @@ def fully_iterated?

private

def batch_size_for_get_more
if batch_size && use_limit?
[batch_size, @remaining].min
else
batch_size
end
end

def exhausted?
limited? ? @remaining <= 0 : false
end
Expand All @@ -405,7 +413,7 @@ def get_more_operation
cursor_id: id,
# 3.2+ servers use batch_size, 3.0- servers use to_return.
# TODO should to_return be calculated in the operation layer?
batch_size: batch_size,
batch_size: batch_size_for_get_more,
to_return: to_return,
max_time_ms: if view.respond_to?(:max_await_time_ms) &&
view.max_await_time_ms &&
Expand Down
16 changes: 16 additions & 0 deletions lib/mongo/operation/map_reduce/result.rb
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,22 @@ def cursor_id
0
end

# Get the number of documents returned by the server in this batch.
#
# Map/Reduce operation returns documents inline without using
# cursors; as such, the standard Mongo::Reply#returned_count does
# not work correctly for Map/Reduce.
#
# Note that the Map/Reduce operation is limited to max BSON document
# size (16 MB) in its inline result set.
#
# @return [ Integer ] The number of documents returned.
#
# @api public
def returned_count
reply.documents.length
end

private

def first_document
Expand Down
5 changes: 1 addition & 4 deletions lib/mongo/operation/result.rb
Original file line number Diff line number Diff line change
Expand Up @@ -238,10 +238,7 @@ def reply
end
end

# Get the count of documents returned by the server.
#
# @example Get the number returned.
# result.returned_count
# Get the number of documents returned by the server in this batch.
#
# @return [ Integer ] The number of documents returned.
#
Expand Down
18 changes: 18 additions & 0 deletions lib/mongo/protocol/msg.rb
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,24 @@ def maybe_add_server_api(server_api)
Msg.new(@flags, @options, main_document, *@sequences)
end

# Returns the number of documents returned from the server.
#
# The Msg instance must be for a server reply and the reply must return
# an active cursor (either a newly created one or one whose iteration is
# continuing via getMore).
#
# @return [ Integer ] Number of returned documents.
def number_returned
if doc = documents.first
if cursor = doc['cursor']
if batch = cursor['firstBatch'] || cursor['nextBatch']
return batch.length
end
end
end
raise NotImplementedError, "number_returned is only defined for cursor replies"
end

private

# Validate that the documents in this message are all smaller than the
Expand Down
30 changes: 30 additions & 0 deletions spec/mongo/cursor_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -543,6 +543,36 @@
end
end
end

context 'when the result set is iterated fully and the cursor id is non-zero' do
min_server_fcv '5.0'

let(:documents) do
(1..5).map{ |i| { field: "test#{i}" }}
end

let(:view) { collection.find(test:{'$gte'=>BSON::MinKey.new}).sort(test:1).limit(5).batch_size(4) }

before do
view.to_a
end

it 'schedules a get more command' do
get_more_commands = subscriber.started_events.select { |e| e.command_name == 'getMore' }
expect(get_more_commands.length).to be 1
end

it 'has a non-zero cursor id on successful get more' do
get_more_commands = subscriber.succeeded_events.select { |e| e.command_name == 'getMore' }
expect(get_more_commands.length).to be 1
expect(get_more_commands[0].reply['cursor']['id']).to_not be 0
end

it 'schedules a kill cursors command' do
get_more_commands = subscriber.started_events.select { |e| e.command_name == 'killCursors' }
expect(get_more_commands.length).to be 1
end
end
end

describe '#inspect' do
Expand Down
41 changes: 41 additions & 0 deletions spec/mongo/protocol/msg_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -525,4 +525,45 @@
end
end
end

describe '#number_returned' do

let(:batch) do
(1..2).map{ |i| { field: "test#{i}" }}
end

context 'when the msg contains a find document' do

let(:find_document) { { "cursor" => { "firstBatch" => batch } } }

let(:find_message) do
described_class.new(flags, options, find_document, *sequences)
end

it 'returns the correct number_returned' do
expect(find_message.number_returned).to eq(2)
end
end

context 'when the msg contains a getmore document' do
let(:next_document) { { "cursor" => { "nextBatch" => batch } } }

let(:next_message) do
described_class.new(flags, options, next_document, *sequences)
end

it 'returns the correct number_returned' do
expect(next_message.number_returned).to eq(2)
end
end

context 'when the msg contains a document without first/nextBatch' do

it 'raises NotImplementedError' do
lambda do
message.number_returned
end.should raise_error(NotImplementedError, /number_returned is only defined for cursor replies/)
end
end
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,7 @@ tests:
command:
getMore: { $$type: [ int, long ] }
collection: *collection0Name
# https://jira.mongodb.org/browse/RUBY-2443
batchSize: 3
batchSize: 1
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test was not consistent with the specs because of how we implemented number_returned. Now that we changed the implementation of it, we can change this to be consistent with the spec tests.

Additionally, it seems that RUBY-2443 can be closed as fixed from this PR. Although, I'm not sure we implemented number_returned exactly how that ticket suggested. (Although, they also seem to default to 0)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@p

commandName: getMore
databaseName: *database0Name
- commandSucceededEvent:
Expand Down