Fix race condition at exit of monitoring filesystem radio #4041
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
khk-globus
approved these changes
Dec 2, 2025
Comment on lines
+51
to
+52
| if exit_event.wait(POLL_PERIOD_S): | ||
| loop = False |
Collaborator
There was a problem hiding this comment.
A do-while approximation. I like it.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Dec 2, 2025
This was uncovered by removal of an unrelated sleep in PR #4037, but I think it will present itself (as missing monitoring data, rather than as an error/exception) in user-facing code. ### Prior to this PR A monitoring message could be written to the new_dir and then shortly after the exit event could be set, sufficiently close in time that the monitoring radio receiver loop exited without seeing those new message files. This PR modifies the exit behaviour of that loop to have one final iteration with the following ordering of events: 1. monitoring messages are written 2. task completes 3. parsl begins to shut down 4. monitoring radio exit event is set by DFK 5. monitoring radio loop observes exit event ### As of this PR 6. monitoring radio loop performs one final processing of directory The new behaviour here is step 6, that a final directory processing will always happen strictly after the exit event is set, which is strictly after the monitoring messages are written in step 1, assuming directories are consistently observable from different places in the filesystem. The misbehaviour can be observed by increasing the delay time of the loop before this PR (for example to 10 seconds) and running the test suite. With this race condition addressed, the loop poll period can be made longer and this PR arbitrarily increases it from 1 second to 10 seconds - although it could also be made configurable. # Changed Behaviour I expect some situations where end of task monitoring data may have been missing to now not be missing that data. ## Type of change - Bug fix
This was uncovered by removal of an unrelated sleep in PR #4037, but I think it will present itself (as missing monitoring data, rather than as an error/exception) in user-facing code. Prior to this PR: A monitoring message could be written to the new_dir and then shortly after the exit event could be set, sufficiently close in time that the monitoring radio receiver loop exited without seeing those new message files. This PR modifies the exit behaviour of that loop to have one final iteration with the following ordering of events: i) monitoring messages are written ii) task completes iii) parsl begins to shut down iv) monitoring radio exit event is set by DFK v) monitoring radio loop observes exit event vi) monitoring radio loop performs one final processing of directory The new behaviour here is step vi, that a final directory processing will always happen strictly after the exit event is set, which is strictly after the monitoring messages are written in step i, assuming directories are consistently observable from different places in the filesystem. The misbehaviour can be observed fairly easily by increasing the delay time of the loop before this PR for example to 10 seconds and running the test suite. With this race condition addressed, the loop poll period can be made longer and this PR arbitrarily increases it from 1 second to 10 seconds - although it could also be made configurable.
khk-globus
force-pushed
the
benc-monitoring-fs-radio-shutdown-race
branch
from
6e776bd to
cdce117
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This was uncovered by removal of an unrelated sleep in PR #4037, but I think it will present itself (as missing monitoring data, rather than as an error/exception) in user-facing code.
Prior to this PR
A monitoring message could be written to the new_dir and then shortly after the exit event could be set, sufficiently close in time that the monitoring radio receiver loop exited without seeing those new message files.
This PR modifies the exit behaviour of that loop to have one final iteration with the following ordering of events:
monitoring messages are written
task completes
parsl begins to shut down
monitoring radio exit event is set by DFK
monitoring radio loop observes exit event
As of this PR
The new behaviour here is step 6, that a final directory processing will always happen strictly after the exit event is set, which is strictly after the monitoring messages are written in step 1, assuming directories are consistently observable from different places in the filesystem.
The misbehaviour can be observed by increasing the delay time of the loop before this PR (for example to 10 seconds) and running the test suite.
With this race condition addressed, the loop poll period can be made longer and this PR arbitrarily increases it from 1 second to 10 seconds - although it could also be made configurable.
Changed Behaviour
I expect some situations where end of task monitoring data may have been missing to now not be missing that data.
Type of change