fix(script): output timing inconsistencies #2667
Conversation
Codecov Report
@@ Coverage Diff @@
## hotfix/3.6.2 #2667 +/- ##
================================================
- Coverage 13.78% 13.77% -0.01%
================================================
Files 153 153
Lines 11274 11282 +8
================================================
Hits 1554 1554
- Misses 9720 9728 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Synchronization had to be moved to the module itself. I don't quite like the way I did it here as it still leaves a possibility for inconsistencies to occur (e.g. if runner calls update handler during get_output execution).
Yes, that's definitely a problem.
The alternative would be to hold lock for the whole execution of get_output. It heavily relies on (only) module::get_output calling get_format, since explicit locking inside get_format would lead to deadlock. Although this implementation would eliminate any inconsistencies, I find it unstable architecture-wise. Maybe I've missed some other obvious solutions here.
I also don't think locking the whole get_output is a good idea here. However, many other modules also rely on module::get_output calling get_format and build, but I agree that that's not very clean.
I think a good way to tackle this is to copy the data struct into a local variable in the beginning of get_output and only use that struct. This resolves all the possible inconsistencies within get_output, but not the one with get_format.
For get_format, we could create a private member variable m_exit_code that we set at the beginning of get_output. This way get_format always relies on the same base-data as get_output.
This would also remove the need for individual get_* methods for all struct members.
The xworkspaces module is an example of another module that sets a member variable in get_output that is later used in a method called by module::get_output (in this case, the build method).
include/adapters/script_runner.hpp
Outdated
| std::atomic_int m_pid{-1}; | ||
| std::atomic_int m_exit_status{0}; | ||
| data m_data; | ||
| bool m_stopping{false}; |
There was a problem hiding this comment.
This still needs to be atomic because the stop function is called from a different thread.
Do you mean something like this? I guess we rely on |
What type of PR is this? (check all applicable)
Description
This PR fixes timing inconsistencies of
scriptmodule output, first described at #2630.Module now receives updates from corresponding runner callback, instead of directly calling
get_*methods of the runner.Synchronization had to be moved to the module itself. I don't quite like the way I did it here as it still leaves a possibility for inconsistencies to occur (e.g. if runner calls update handler during
get_outputexecution).The alternative would be to hold lock for the whole execution of
get_output. It heavily relies on (only)module::get_outputcallingget_format, since explicit locking insideget_formatwould lead to deadlock. Although this implementation would eliminate any inconsistencies, I find it unstable architecture-wise. Maybe I've missed some other obvious solutions here.Related Issues & Documents
Closes #2650
Documentation (check all applicable)