Signal timeout when locking task in getLongRunningOpCalls #8511
Conversation
Summary: If there are some problem (e.g. deadlock) with the mutex in `Task`, `getLongRunningOpCalls` would block forever waiting for it. In this case we should signal the error and let the caller to generate some form of alerts. This is important as we rely on `getLongRunningOpCalls` to detect blocking and the function itself should not block. Differential Revision: D53048088
✅ Deploy Preview for meta-velox canceled.
|
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D53048088 |
| @@ -194,7 +194,7 @@ void BlockingState::setResume(std::shared_ptr<BlockingState> state) { | |||
| auto& driver = state->driver_; | |||
| auto& task = driver->task(); | |||
|
|
|||
| std::lock_guard<std::mutex> l(task->mutex()); | |||
| std::lock_guard<std::timed_mutex> l(task->mutex()); | |||
There was a problem hiding this comment.
@Yuhta we do see the problem in the production? Thanks!
There was a problem hiding this comment.
No it's for preventive measure, first to rule out the possibility that we get deadlock here during the detection (collecting run time of op call)
| /// Return false when the lock cannot be taken within the timeout, in that | ||
| /// case the result is not populated. Return true if everything works well. | ||
| bool getLongRunningOpCalls( | ||
| std::chrono::nanoseconds lockTimeout, |
There was a problem hiding this comment.
What is the timeout? Can't find where is it specific for some reason.
There was a problem hiding this comment.
It's the timeout for locking the per task mutex. I will update the comment to make it more explicit.
There was a problem hiding this comment.
But what value do we use for this timeout? Where can I see that?
There was a problem hiding this comment.
It will be in D52921877 but that is still work in progress. I am planning to use 10s.
|
This pull request has been merged in 147ff30. |
|
Conbench analyzed the 1 benchmark run on commit There weren't enough matching historic benchmark results to make a call on whether there were regressions. The full Conbench report has more details. |
Summary:
If there are some problem (e.g. deadlock) with the mutex in
Task,getLongRunningOpCallswould block forever waiting for it. In this case weshould signal the error and let the caller to generate some form of alerts.
This is important as we rely on
getLongRunningOpCallsto detect blocking andthe function itself should not block.
Differential Revision: D53048088