[SERVER-26526] make TaskExecutor::cancel return whether the task was successfully canceled Created: 07/Oct/16  Updated: 08/Jan/24  Resolved: 31/Oct/16

Status: Closed
Project: Core Server
Component/s: Networking
Affects Version/s: 3.4.0-rc0
Fix Version/s: None

Type: Improvement Priority: Major - P3
Reporter: Esha Maharishi (Inactive) Assignee: Mira Carey
Resolution: Done Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Related
related to SERVER-26524 check if addShardHandle present in Sh... Closed
Backwards Compatibility: Fully Compatible
Sprint: Platforms 2016-10-31
Participants:

 Description   

In at least one case (SERVER-26524), we had to add a brittle workaround because canceling a task does not guarantee that CallbackCanceled is delivered to the callback (if the task completes before cancel() is called, the actual response will be delivered instead).

I see how it's difficult to directly return whether CallbackCanceled will be delivered since the task is canceled by post()'ing to the asio::io_service::strand:

https://github.com/mongodb/mongo/blob/r3.4.0-rc0/src/mongo/executor/thread_pool_task_executor.cpp#L360 ->
https://github.com/mongodb/mongo/blob/r3.4.0-rc0/src/mongo/executor/network_interface_asio.cpp#L427 ->
https://github.com/mongodb/mongo/blob/r3.4.0-rc0/src/mongo/executor/network_interface_asio_operation.cpp#L114-L122

but perhaps a blocking version of TaskExecutor::cancel (or a flag that indicates whether it should block) could be added?



 Comments   
Comment by Mira Carey [ 26/Oct/16 ]

That's reasonable. I think that, for 3.6, we're likely to take another stab at the TaskExecutor API to make events more like futures (so you could pull things like cancellation out of waiting on them). But for now, go ahead and use the wait to fill in a RemoteCommandResponse.

Comment by Esha Maharishi (Inactive) [ 26/Oct/16 ]

That's fine; is there any way after waiting on the callback, to check if CallbackCanceled was delivered?

One way I've seen this done is to create a RemoteCommandResponse variable before scheduling the task, capture that variable into the callback, and assign the RemoteCommandCallbackArgs.response to it within the callback.

Then after waiting for the callback, we can examine that captured variable.

https://github.com/mongodb/mongo/blob/r3.4.0-rc1/src/mongo/s/client/shard_remote.cpp#L203-L220
(Here, we check for ExceededTimeLimit, but we could also check for CallbackCanceled).

Does this seem like a good general pattern? (It's not the way I wrote the workaround in SERVER-26524, but it could be changed to be written that way).

mira.carey@mongodb.com

Comment by Mira Carey [ 26/Oct/16 ]

A synchronous cancel() implies fully blocking on the operation until it's completely resolved. Given that some cancelers won't want to block, we already have to design our callback functions to handle cancellation. Which means we're going to have to deal with the cancel in two places, ensure handling code doesn't race and deal with the ambiguity of where it lives (synchronous canceler is on thread, async callback isn't).

I'm leaning Andy's way. Make your callback handle the cancellation and wait on the callback. The alternative (making your callback only sometime run) is going to cause you more headache than time saved.

Comment by Esha Maharishi (Inactive) [ 10/Oct/16 ]

Although, even if can't be done in a non-blocking way, it's maybe less complicated and more easily testable to have TaskExecutor::cancel() return whether it succeeded... than make every person who codes a callback manage concurrent access to a variable or data structure for this purpose?

Comment by Esha Maharishi (Inactive) [ 10/Oct/16 ]

That's a good point; I guess if there was a non-blocking way for TaskExecutor::cancel() to return whether CallbackCanceled would be set on the callback or if it was too late, that would be preferable. Maybe this could be done by checking what stage/state the task was in?

If it can't be done in a non-blocking way, waiting on the callback is equivalent.

Comment by Andy Schwerin [ 10/Oct/16 ]

Why not just wait on the callback handle, if you're willing to block, and have the callback's own behavior signal canceledness to the observer, eg by setting a flag?

Comment by Esha Maharishi (Inactive) [ 07/Oct/16 ]

We could also consider doing this as part of PM-642.

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