[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: |
|
||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||
| Sprint: | Platforms 2016-10-31 | ||||||||
| Participants: | |||||||||
| Description |
|
In at least one case ( 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 -> 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 Does this seem like a good general pattern? (It's not the way I wrote the workaround in |
| 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. |