-
Notifications
You must be signed in to change notification settings - Fork 614
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(system_monitor): multithreading support for boost::process #1714
fix(system_monitor): multithreading support for boost::process #1714
Conversation
Signed-off-by: v-nakayama7440-esol <v-nakayama7440@esol.co.jp>
@v-nakayama7440-esol Could you please provide some measurements such as visualized interval of before and after the fix in Tests performed above? |
@ito-san |
Codecov Report
@@ Coverage Diff @@
## main #1714 +/- ##
==========================================
- Coverage 10.67% 10.61% -0.06%
==========================================
Files 1109 1109
Lines 78619 79029 +410
Branches 18587 18587
==========================================
Hits 8392 8392
- Misses 61312 61722 +410
Partials 8915 8915
*This pull request uses carry forward flags. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…arefoundation#1714) Signed-off-by: v-nakayama7440-esol <v-nakayama7440@esol.co.jp> Signed-off-by: v-nakayama7440-esol <v-nakayama7440@esol.co.jp>
…arefoundation#1714) Signed-off-by: v-nakayama7440-esol <v-nakayama7440@esol.co.jp> Signed-off-by: v-nakayama7440-esol <v-nakayama7440@esol.co.jp>
…arefoundation#1714) Signed-off-by: v-nakayama7440-esol <v-nakayama7440@esol.co.jp> Signed-off-by: v-nakayama7440-esol <v-nakayama7440@esol.co.jp>
…arefoundation#1714) Signed-off-by: v-nakayama7440-esol <v-nakayama7440@esol.co.jp> Signed-off-by: v-nakayama7440-esol <v-nakayama7440@esol.co.jp>
…arefoundation#1714) Signed-off-by: v-nakayama7440-esol <v-nakayama7440@esol.co.jp> Signed-off-by: v-nakayama7440-esol <v-nakayama7440@esol.co.jp>
…arefoundation#1714) Signed-off-by: v-nakayama7440-esol <v-nakayama7440@esol.co.jp> Signed-off-by: v-nakayama7440-esol <v-nakayama7440@esol.co.jp>
…arefoundation#1714) Signed-off-by: v-nakayama7440-esol <v-nakayama7440@esol.co.jp> Signed-off-by: v-nakayama7440-esol <v-nakayama7440@esol.co.jp>
…arefoundation#1714) (#170) Signed-off-by: v-nakayama7440-esol <v-nakayama7440@esol.co.jp> Signed-off-by: v-nakayama7440-esol <v-nakayama7440@esol.co.jp> Signed-off-by: v-nakayama7440-esol <v-nakayama7440@esol.co.jp> Co-authored-by: v-nakayama7440-esol <97144416+v-nakayama7440-esol@users.noreply.github.com>
Signed-off-by: v-nakayama7440-esol v-nakayama7440@esol.co.jp
Description
boost::process::pipe internally creates file descriptor. However, this file descriptor does not support multithreading because O_CLOEXEC is not set. Because of this, when each thread of SystemMontor uses boost::process to create a child process, the file descriptors of boost::process::pipe conflict between child processes. As a result, the child process waits for the file descriptor to be released.
In this PR, the SystemMonitor application creates file descriptor with O_CLOEXEC set and passes it to boost::process::pipe to eliminate the wait for child process to release file descriptor.
Related links
Tests performed
Process Monitor's /diagnostics topic message timestamp interval is essentially about 1 second (diagnostic_updater event execution cycle is 1hz). However, before the fix, the interval would occasionally be about 2 seconds.
Graph before fix (vertical axis unit is msec, horizontal axis unit is message order):
![image](https://user-images.githubusercontent.com/97144416/187135812-acd9ef0f-676f-4845-9577-c1103a8075e4.png)
After the fix, Process Monitor confirmed that the timestamp interval for messages on the /diagnostics topic was not always reaching 2 seconds. Monitors other than Process Monitor were also checked in the same way.
Graph after fix (vertical axis unit is msec, horizontal axis unit is message order):
![image](https://user-images.githubusercontent.com/97144416/187135945-f43a9023-d77b-4cb4-b38c-a31edef75c74.png)
![image](https://user-images.githubusercontent.com/97144416/187137405-7eac3cba-21ec-43ef-a768-82f643f19188.png)
Additional tests performed
In order to check if no side effects come by this fix, verified the all of
/diagnostics
topics are sent periodically and notified as OK during normal operation.Notes for reviewers
Pre-review checklist for the PR author
The PR author must check the checkboxes below when creating the PR.
In-review checklist for the PR reviewers
The PR reviewers must check the checkboxes below before approval.
Post-review checklist for the PR author
The PR author must check the checkboxes below before merging.
After all checkboxes are checked, anyone who has write access can merge the PR.