[JAVA-3907] AsyncQueryBatchCursor does not release ConnectionSource when closed Created: 10/Dec/20 Updated: 28/Oct/23 Resolved: 04/Feb/21 |
|
| Status: | Closed |
| Project: | Java Driver |
| Component/s: | Reactive Streams |
| Affects Version/s: | None |
| Fix Version/s: | 4.2.1 |
| Type: | Bug | Priority: | Major - P3 |
| Reporter: | Jeffrey Yemin | Assignee: | Valentin Kavalenka |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| Description |
|
AsyncQueryBatchCursor#close does not release its reference to the ConnectionSource. This is not a problem if iteration completes successfully, as the getMore callback releases it when the server cursor is exhausted. But if iteration is interrupted due to error or the cursor is not fully iterated by the application, the reference will never get released. This can cause a leak, as a ConnectionSource can hold a reference to a resource (most likely a ClientSession. |
| Comments |
| Comment by Githook User [ 05/Apr/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Valentin Kovalenko', 'email': 'valentin.kovalenko@mongodb.com', 'username': 'stIncMale'}Message: Backport to 4.2.x
Ensures that all sessions are returned to the pool
Before the changes made within The approach with using startAtOperationTime to ensure that Co-authored-by: Ross Lawley <ross.lawley@gmail.com> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 08/Feb/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Ross Lawley', 'email': 'ross.lawley@gmail.com', 'username': 'rozza'}Message: Regression test for change stream cancellation (#661) Ensures that all sessions are returned to the pool
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Githook User [ 04/Feb/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Author: {'name': 'Valentin Kovalenko', 'email': 'valentin.kovalenko@mongodb.com', 'username': 'stIncMale'}Message: Make implementations of AsyncAggregateResponseBatchCursor more responsive to close signals (#654) 1) Change implementations of AsyncAggregateResponseBatchCursor to make them more responsive to close signals AsyncAggregateResponseBatchCursor wraps AsyncQueryBatchCursor. 2) Release resources in AsyncQueryBatchCursor The changes made is this commit appear to be correct from the standpoint of tests,
The following text expresses my thoughts about reference counting and is not directly "Structured" reference counting. In simple words, the following As a side note, structured programming applied to concurrent code As much as I would like us to use the structured approach, it is generally speaking Consider the following legacy code in which we want to refactor releaseEarlyLegacyMethod void releaseEarlyLegacyMethod() { ReferenceCounted resource = pool.borrow();//count is 1 /* Since ReleaseEarlyLegacyConsumer is the last party using the resource, it must release the resource. * A reader cannot find the release call corresponding to the borrow call inside the releaseEarlyLegacyMethod method.*/ releaseEarlyUnchangedCode(new ReleaseEarlyLegacyOneTimeConsumer(resource)); } class ReleaseEarlyLegacyOneTimeConsumer implements Runnable { ReleaseEarlyLegacyOneTimeConsumer(ReferenceCounted resource) { this.resource = resource;//count is 1 } public void run() { finally {//release the resource because ReleaseEarlyLegacyConsumer.accept is the last party accessing it /* Note that by looking at this release call, a reader cannot easily find the corresponding retain/borrow call. * The corresponding retain/borrow call is not even in this class.*/ resource.release();//count is 0; the resource returns to the pool } } void releaseEarlyUnchangedCode(Runnable action) { finally {//release the resource because releaseEarlyUnchangedCode is the last party accessing it resource.release();//count is 0; the resource is returned to the pool }} The pool.borrow call in releaseEarlyUnchangedCode works because the resource is returned to the pool before action.run() returns. void structuredNewMethod() { finally { resource.release();//count is 0; the resource returns to the pool }} class StructuredNewOneTimeConsumer implements Runnable { StructuredNewOneTimeConsumer(ReferenceCounted resource) { this.resource = resource.retain();//count is 2; we store a new reference to the resource } public void run() { finally {//release the resource because StructuredNewConsumer is not going to use it anymore /* The corresponding retain call is in the constructor of the class because resource is the instance field. * A reader may easily locate it.*/ resource.release();//count is 1 } } With this new code, the pool.borrow call in releaseEarlyUnchangedCode fails because the resource is not returned to the pool before action.run() returns. This would not be a problem if the code was written following the structured approach from the beginning
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Valentin Kavalenka [ 27/Jan/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Scenario 2)Changing AsyncQueryBatchCursor.handleGetMoreQueryResult alone to react to pending cancel does not help because AsyncQueryBatchCursor.close is not called to set pending cancel when Subscription.cancel is called. This happens due to the pending operation/cancel logic in AsyncChangeStreamBatchCursor that wraps AsyncQueryBatchCursor. My next step is to understand the pending operation/cancel logic in AsyncChangeStreamBatchCursor and see what can be done about it. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Ross Lawley [ 26/Jan/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
That sounds like a bug / unintentional change in behaviour. Added | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Jeffrey Yemin [ 26/Jan/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
ross.lawley do you recall if it is intentional that change stream cursors establish themselves before any documents are requested? Did we think that was necessary in order to establish the resume token? I don't recall. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Valentin Kavalenka [ 26/Jan/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Improve the memo in the Quick Start PrimerCurrently it states:
The actual behavior of the ChangeStreamPublisher is:
It may be more useful to say
It may also be useful to duplicate the full memo (both the "cold" part and the "unicast" part) in the documentation of the com.mongodb.reactivestreams.client package (it contains our Publisher classes). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Valentin Kavalenka [ 22/Jan/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
The following example is inspired by the example in
The relevant shortened output:
Writing it and playing with it was useful for my understanding of the problem and the expected behavior. There are three scenarios I tried: Scenario 2)Seems to be caused by The correct behaviour is to cancel the pending operation and release all resources (I was unable to find any clear requirement to call Subscriber.onComplete in this scenario, and I interpret this as that calling the method is optional; relevant links: Reactor Reference Guide - Flux, org.reactivestreams.Subscriber and tangentially java.util.concurrent.Flow.Subscription.cancel - "subscription need not ever receive an onComplete or onError signal"). Cancelling the pending operation boils down to cancelling pending com.mongodb.connection.Stream.readAsync, which can be done only by calling Stream.close. Therefore, I think the following handling of Subscription.cancel is how the driver should ideally behave (it is possible that at this point I am missing some details, but I believe the outline is correct):
None of the reactive driver operations currently behave as described above, instead they all handle cancel signals by remembering them (pending cancel) and acting later when a pending action related to the corresponding subscription returns. We will not refactor the implementation to use the aforementioned approach, instead we will only fix the AsyncQueryBatchCursor, which does not act on the cancel signal even when a pending getMore returns1, unless it produces any results (see AsyncQueryBatchCursor.handleGetMoreQueryResult). Further discussion of the bug we have in this scenario and its solution continues in the comments below. Scenario 3)We need Reactor to notify the driver when it decides that a subscription is terminated as a result of Subscriber.onNext throwing an exception. It appears that Flux.onError* methods (and maybe Flux.doOnTerminate, Flux.doOnError) are the levers we need to pull for achieving the desired behavior. However, I did not have any success as a result of playing with using these methods in BatchCursorPublisher.subscribe. Currently this problem looks like a rabbit hole that deserves its own bug report. 1 One may also imagine releasing the borrowed connection when getMore returns without any results and there is no pending cancel event, to give other tasks a chance to use the connection. We are not going to do such a change. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Comment by Ross Lawley [ 15/Jan/21 ] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
I think |