[SERVER-37453] Delete PlanStage::dispose() Created: 03/Oct/18 Updated: 29/Oct/23 Resolved: 27/Aug/20 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | Querying |
| Affects Version/s: | None |
| Fix Version/s: | 4.7.0 |
| Type: | Task | Priority: | Major - P3 |
| Reporter: | David Storch | Assignee: | David Storch |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | todo_in_code | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||||||||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||||||||||||||
| Sprint: | Query 2019-02-25, Query 2020-09-07 | ||||||||||||||||||||
| Participants: | |||||||||||||||||||||
| Description |
|
Once all cursors are globally managed ( |
| Comments |
| Comment by Githook User [ 27/Aug/20 ] | |||
|
Author: {'name': 'David Storch', 'email': 'david.storch@mongodb.com', 'username': 'dstorch'}Message: | |||
| Comment by David Storch [ 25/Aug/20 ] | |||
|
Although the above discussion about why dispose() cannot be eliminated from the PlanExecutor interface still holds, there is no longer any use for PlanStage::dispose() due to the changes in | |||
| Comment by Githook User [ 06/Nov/19 ] | |||
|
Author: {'username': 'antkorsh', 'email': 'anton.korshunov@mongodb.com', 'name': 'Anton Korshunov'}Message: | |||
| Comment by Anton Korshunov [ 13/Feb/19 ] | |||
|
The purpose of the dispose mechanism in the PlanExecutor/ClientCuror was to:
Since the dispose mechanism requires access to the current OperationContext and, before "All cursors globally managed" project, could acquire a lock to deregister its PlanExecutor, it wasn't feasible to implement the dispose mechanics through the means of a class destructor. Firstly, because acquiring a lock in destructor may fail and there is no a clean way to propagate this failure to the caller (as throwing an exception from a destructor is illegal and likely will result in abnormal program termination), and secondly, a destructor may not had access to the current OperationContext. The dispose mechanism solves these problems and also provides a mechanics to automatically call the dispose() method by utilizing the concept of unique_ptr deleters. In this work item we tried to get rid of the PlanExecutor::dispose() methods and its friends, driven by the idea that since the lock acquision doesn't come into play anymore with the introduction of the cursor manager scoped to the ServiceContext, rather than a collection, we may not need any longer the dispose mechanics. However, while implementing this change we came across a scenario where the dispose mechanism still stands out and proves to be an efficient (if not the only) way to deal with killCursors requests. Namely, DocumentSourceMergeCursors relies on the Pipeline::dispose() mechanism to thread the dispose call down as PlanExecutor -> PipelineProxy -> Pipeline -> DocumentSourceMergeCursors. If the dispose() is not called on DocumentSourceMergeCursors, the cursor may not get killed and we may end up with a situation when the async result merger invariant doesn't hold. This can be illustrated with the following test: Not properly performing the dispose step on DocumentSourceMergeCursors results in the invariant being violated in AsyncResultsMerger::~AsyncResultsMerger():
So, for $mergeCursors we still need a valid OperationContext to attach to, and we need to ensure the dispose() methods gets called, so both the dispose() mechanism and a custom deleter would be hard to be replaced in this case. Another potential problem (although not so critical as with the DocumentSourceMergeCursors) is DocumentSourceFacet where we need to propage dispose() down to the source document. The following test would fail if this is not done properly:
In light of the above, it was decided to keep the dispose mechanism unchanged as it's still useful in certain cases and would be hard to be replaced with some other approach. | |||
| Comment by David Storch [ 06/Feb/19 ] | |||
|
anton.korshunov, since this is closely related to work I've been doing for the "all cursors globally managed" project, please add me to any code review you produce. Thanks! Also, I'm happy to help if you have any questions or run into any issues. |