Fix coverity issues. Do not re-throw worker thread error in the destructor. #3886
Conversation
stiepan
force-pushed
the
coverity_april_22
branch
from
1d939df
to
3b1e312
Compare
Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
stiepan
force-pushed
the
coverity_april_22
branch
from
3b1e312
to
3a83985
Compare
|
CI MESSAGE: [4800120]: BUILD STARTED |
| std::unique_lock<std::mutex> lock(mutex_); | ||
| while (!work_complete_) { | ||
| completed_.wait(lock); | ||
| } | ||
|
|
||
| // Check for errors | ||
| if (!errors_.empty()) { | ||
| if (rethrow_worker_errors && !errors_.empty()) { |
There was a problem hiding this comment.
Are we okay not to break the execution if WaitForWork(false) and there is an error?
There was a problem hiding this comment.
I think that was the idea, wasn't it? Ideally, we would break if there was something other than runtime_error.
There was a problem hiding this comment.
If you refer to running_ = false and cv_.notify I don't think it does anything here: only the worker thread sets the errors and if so, it stops execution anyway. As to not throwing, I understood that it is the idea as @mzient says.
|
CI MESSAGE: [4800120]: BUILD PASSED |
dali/pipeline/util/worker_thread.h
Outdated
| @@ -89,10 +89,10 @@ class WorkerThread { | |||
| * When the destructor is called other things that work() is using may have been gone long | |||
| * before causing a hang. Now when Shutdown is called we are sure that all things around still exist. | |||
| */ | |||
| inline void Shutdown(void) { | |||
| inline void Shutdown(bool rethrow_worker_errors = true) { | |||
There was a problem hiding this comment.
Do we have a legitimate case for rethrowing in shutdown? Isn't it ultimately always called from some kind of destructor?
There was a problem hiding this comment.
Yeah, one way or another it is always called from destructors.
dali/pipeline/util/worker_thread.h
Outdated
| @@ -119,14 +119,14 @@ class WorkerThread { | |||
| cv_.notify_one(); | |||
| } | |||
|
|
|||
| inline void WaitForWork() { | |||
| inline void WaitForWork(bool rethrow_worker_errors = true) { | |||
| std::unique_lock<std::mutex> lock(mutex_); | |||
| while (!work_complete_) { | |||
There was a problem hiding this comment.
Btw. it seems that calling WaitForWork while worker actually does something and then it raises the error, may deadlock as the worker thread does not set¬ify completed_.
Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
|
CI MESSAGE: [4807109]: BUILD STARTED |
|
CI MESSAGE: [4807109]: BUILD PASSED |
…uctor. (NVIDIA#3886) * Prevents throwing exception from the worker thread in the destructor * Fixes possible deadlock on Shutdown/WaitForWork Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
…uctor. (NVIDIA#3886) * Prevents throwing exception from the worker thread in the destructor * Fixes possible deadlock on Shutdown/WaitForWork Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>
Signed-off-by: Kamil Tokarski ktokarski@nvidia.com
Category:
Description:
Adds extra parameter to WaitForWork method to prevent it from re-throwing errors in the destructor.
Additional information:
Affected modules and functionalities:
Fixes possible deadlock on Shutdown if the worker thread quit with the execption
Key points relevant for the review:
Checklist
Tests
Documentation
DALI team only
Requirements
REQ IDs: N/A
JIRA TASK: DALI-2736