AsyncCommandBatchCursor now uses a ResourceManager

[rozza]

Part of a wider Command Cursor refactoring

JAVA-5159

[stIncMale]

releaseServerAndClientResources in CommandBatchCursor simply calls releaseServerResources followed by releaseClientResources in the finally block. Is there something that makes it impossible to implement the asynchronous releaseServerAndClientResources in the same straightforward way? (the code will still be more cumbersome because of releaseClientResources being asynchronous, as mentioned in #399 (comment))

[stIncMale]

This method is asynchronous, but its signature makes it look as if it were synchronous: the method does not accept a callback, doesnot return afuture, etc. This makes the methods releaseServerResources, releaseServerAndClientResources also "secretly" asynchronous. This means that any code that is executed in CommandBatchCursor after, in the program order (PO), the aforementioned methods, is executed concurrently with them in AsyncCommandBatchCursor.

Such a behavioral change may obviously cause bugs in a general case. Specifically here, it seems to have a potential to violate the following crucial guarantee ResourceManager is meant to provide:

It also implements a {@linkplain #doClose() deferred close action} such that it is totally ordered with other operations of {@link AsyncCommandBatchCursor} (methods {@link #tryStartOperation()}/{@link #endOperation()} must be used properly to enforce the order) despite the method {@link AsyncCommandBatchCursor#close()} being called concurrently with those operations.

I haven't checked whether this hidden asynchrony actually causes a bug, but the code is already difficult enough that adding more asynchrony should probably not be done lightly, especially given that this makes a behavioral difference with the synchonous code. Also, even if this asynchrony does not cause any bugs, it is risky to have it because it is hidden. I propose to make all "secretly" asynchronous methods explicitly asynchronous, and write the rest of the code taking that asynchrony into account.

[rozza]

Ack - yeah makes sense will look to fix in #401

[rozza]

Just to note: doClose is guarded itself not close. Even in the sync world it would be possible to call close outside tryStartOperation/endOperation.

[stIncMale]

The reason tryStartOperation, endOperation are private is that they are not supposed to be called outside of ResourceManager. In CommandBatchCursor, these methods are an implementation detail of ResourceManager.execute. All "operations" are then clearly demarcated by this execute method, which is what I would expect to see in AsyncCommandBatchCursor if it were refactored to rely on ResourceManager similarly to CommandBatchCursor.

Naturally, AsyncCommandBatchCursor.ResourceManager.execute can't be implemented using the try-finally block, as it is done in CommandBatchCursor. Instead, an approach similar to Locks.withLockAsync (see mongodb#1134), or to AsyncOperationHelper.executeRetryableReadAsync (which is conceptually the same thing, with a different API and a bit different behavior) has to be used.

The current approach does not demarcate operations clearly and makes it difficult to see if each invocation of tryStartOperation is guaranteed to be followed by a corresponding endOperation. For example, AsyncCommandBatchCursor.next is supposed to be a single "operation" from the standpoint of ResourceManager, and it calls tryStartOperation once, but endOperation is called in many places. I would expect endOperation to be at least called every time we complete the callback passed to AsyncCommandBatchCursor.next, but this does not seem to be the case: for example, getMore(final ServerCursor cursor, final SingleResultCallback<List<T>> callback) does not call endOperation if t != null (should this be fixed, or am I missing something?).

[rozza]

Lots of feedback, which is great but it's important to note this is a partial commit that was split out of another PR mongodb#1198 and alot of the code is refactored so its shared in #401

The original code was written over a month ago, so some reasons for divergance have been lost and there was no need to modify CommandBatchCursor in this set of PRs prior to the unification changes in #401

I would recommend addressing #401 over changes here, if we are happy with the premise that the Async code also uses a resource manager for handling of resources.

[rozza]

Closed and rolled up to #401