Abstract Cursor Resource Manager

[rozza]

Two commits:

1. Auto refactor of the CursorResourceManager - into its own class.
2. Made CursorResourceManager abstract and then implemented it in both sync and async.

You should be able to view both implementations side by side.

Note, the aim isn't to make the API's / behaviour exactly same - rather its to bring them to a point where they are similar enough, that it isn't such a mental leap to work on them.

[rozza]

Note: This follows on from the previous AsyncSingleBatchCursor change where next() no longer returns a null value.

[rozza]

I think we can remove the auto-closing nature of these AsyncBatchCursors now.

I propose adding a hasNext method which just then returns resourceManager.getServerCursor() != null for async.

[jyemin]

I commented on what I think is the same change in the sync version, where it's easier to understand.

[rozza]

This is different to the sync issue (or I missed which one was commented on). AsyncBatchCursors automatically mark themselves as closed on consumption of all resources. That predates these refactorings.

[jyemin]

I think my comment on the sync version is about something different, so ignore my previous comment on this thread.

[jyemin]

I think what you're saying is that users of the batch cursors will now have to call close explicitly in order to release resources (i.e the pinned connection and the connection source). If so, that is a behavioral change that we have to consider carefully, as it effects not just driver internals but users of the driver. Consider what we've allowed in the legacy API:

DBCursor cursor = dbCollection.find();
while (cursor.hasNext()) {
   cursor.next();
}
// allow cursor to go out of scope and let the cursor cleaner call DBCursor#close, which in turn calls CommandBatchCursor#close

Currently, if the above loop exits normally (without throwing), all the resources will be released and the cursor cleaner is a no-op. With this proposed change, the releases won't be released until GC kicks in.

While the above is shoddy application coding, we have allowed it and I'm hesitant to change its behavior unless there is a compelling reason.

[rozza]

Ah that makes more sense, let me capture that in a test and apply the fix to release.

[rozza]

nm - there are tests that already cover this scenario for example:

should handle getMore when there are empty results but there is a cursor

So there is no change in behaviour here - because setting the server cursor to null resourceManger.setServerCursor already releases the resources automatically if the cursor is null.

[stIncMale]

I agree that the code

if (getServerCursor() == null) {
    close();
}

is not needed for releasing resources (and there is no such code in CommandBatchCursor), but it is needed to self-close the AsyncCommandBatchCursor. As far as I understand, without this code the AsyncCommandCatchCursor.isClosed will continue returning false despite all resources having been released (CommandBatchCursor does not have this issue because it does not have the isClosed method).

Thus, I think we should live the code as is and add a comment explaining what's going on.

[stIncMale]

Even better, instead of doing

if (getServerCursor() == null) {
    close();
}

after endOperation in AsyncCommandBatchCursor, let's just call close instead of releaseClientResources in CursorResourceManager.setServerCursor (the code change is suggested in #404 (comment)). This will have the same effect, and AsyncCommandBatchCursor won't need the aforementioned additional code that we don't have in CommandBatchCursor.

[rozza]

Cannot close in CursorResourceManager.setServerCursor as auto closing breaks the sync cursor.

[jyemin]

The ChangeStreamBatchCursorSpecification#should call the underlying CommandBatchCursor is failing for me locally with

com.mongodb.MongoException
	at app//com.mongodb.MongoException.fromThrowableNonNull(MongoException.java:84)
	at app//com.mongodb.internal.operation.ChangeStreamBatchCursor.resumeableOperation(ChangeStreamBatchCursor.java:192)
	at app//com.mongodb.internal.operation.ChangeStreamBatchCursor.next(ChangeStreamBatchCursor.java:79)
	at com.mongodb.internal.operation.ChangeStreamBatchCursorSpecification.should call the underlying CommandBatchCursor(ChangeStreamBatchCursorSpecification.groovy:49)
Caused by: java.lang.AssertionError
	at com.mongodb.assertions.Assertions.assertNotNull(Assertions.java:181)
	at com.mongodb.internal.operation.ChangeStreamBatchCursor.convertAndProduceLastId(ChangeStreamBatchCursor.java:174)
	at com.mongodb.internal.operation.ChangeStreamBatchCursor.lambda$next$2(ChangeStreamBatchCursor.java:81)
	at com.mongodb.internal.operation.ChangeStreamBatchCursor.resumeableOperation(ChangeStreamBatchCursor.java:189)
	... 2 more

[jyemin]

I commented on what I think is the same change in the sync version, where it's easier to understand.

[stIncMale]

Reviewed CommandBatchCursor and CursorResourceManager with regard to how it is used in the synchronous code.

[stIncMale]

Partially reviewed AsyncCommandBatchCursor.

[stIncMale]

There is an optional suggestion, otherwise approved.