[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:
Related
related to SERVER-35769 Pointer to freed OperationContext is ... Closed
related to SERVER-39392 Invariant in PlanStage::dispose alway... Closed
related to SERVER-43448 Complete TODO listed in SERVER-37453 Closed
related to SERVER-44203 Complete TODO listed in SERVER-37453 Closed
Backwards Compatibility: Fully Compatible
Sprint: Query 2019-02-25, Query 2020-09-07
Participants:

 Description   

Once all cursors are globally managed (SERVER-37451) the dispose() mechanism for cleaning up ClientCursor and PlanExecutor will no longer be needed.



 Comments   
Comment by Githook User [ 27/Aug/20 ]

Author:

{'name': 'David Storch', 'email': 'david.storch@mongodb.com', 'username': 'dstorch'}

Message: SERVER-37453 Delete PlanStage::dispose()
Branch: master
https://github.com/mongodb/mongo/commit/6663d804ebf22ec5c4295d082ad3685bf801da6b

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 SERVER-48478. I'm repurposing this ticket to be specifically about deleting PlanStage::dispose().

Comment by Githook User [ 06/Nov/19 ]

Author:

{'username': 'antkorsh', 'email': 'anton.korshunov@mongodb.com', 'name': 'Anton Korshunov'}

Message: SERVER-44203 Complete TODO listed in SERVER-37453
Branch: master
https://github.com/mongodb/mongo/commit/e7828e2249c1d0b97ff981ce3d228648bdf02a52

Comment by Anton Korshunov [ 13/Feb/19 ]

The purpose of the dispose mechanism in the PlanExecutor/ClientCuror was to:

  1. Ensure that a collection lock is acquired while detaching a PlanExecutor from the CursorManager.
  2. Take responsibility to invoke dispose() methods before destruction of these objects, so that the caller doesn't need to care about it.

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:

https://github.com/mongodb/mongo/blob/530a26bc5387de3dd131a18801a6c3253c4f3220/src/mongo/db/pipeline/document_source_merge_cursors_test.cpp#L296

Not properly performing the dispose step on DocumentSourceMergeCursors results in the invariant being violated in AsyncResultsMerger::~AsyncResultsMerger():

invariant(_remotesExhausted(lk) || _lifecycleState == kKillComplete)

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:

https://github.com/mongodb/mongo/blob/a66a5578d5b006cef85b16eac05c96b58c877ebe/src/mongo/db/pipeline/document_source_facet_test.cpp#L411

facetStage->dispose();
ASSERT_TRUE(mockSource->isDisposed);   

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.

Generated at Thu Feb 08 04:46:02 UTC 2024 using Jira 9.7.1#970001-sha1:2222b88b221c4928ef0de3161136cc90c8356a66.