-
Notifications
You must be signed in to change notification settings - Fork 32
🐛 Avoids possible early garbage collection of task #8121
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
🐛 Avoids possible early garbage collection of task #8121
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #8121 +/- ##
===========================================
- Coverage 89.83% 67.05% -22.78%
===========================================
Files 1695 725 -970
Lines 65858 33422 -32436
Branches 814 176 -638
===========================================
- Hits 59165 22412 -36753
- Misses 6480 10952 +4472
+ Partials 213 58 -155
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
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.
Pull Request Overview
This PR fixes a potential issue where a follow-up task could be garbage collected early by retaining a reference to it and ensuring it's properly canceled.
- Introduces
_task_uploading_followupto hold the cleanup task created in the upload-done callback. - Updates the remove-downloads callback to assign the task to the new attribute.
- Extends the cancellation routine to handle and clear the follow-up task.
Comments suppressed due to low confidence (3)
services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/outputs/_manager.py:121
- [nitpick] The name
_task_uploading_followupis somewhat vague. Consider renaming it to something like_task_remove_all_uploadingor_cleanup_upload_taskfor clearer intent.
self._task_uploading_followup: Task | None = None
services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/outputs/_manager.py:175
- It would help future maintainers to add a comment here explaining why we retain this task reference (to prevent early garbage collection in the upload-done callback).
self._task_uploading_followup = create_task(
services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/outputs/_manager.py:186
- There’s no existing test covering the cancellation of the follow-up task. Consider adding unit tests for
_uploading_task_cancelto verify both_task_uploadingand_task_uploading_followupare canceled and cleared correctly.
if self._task_uploading_followup is not None:
sanderegg
left a comment
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.
thanks
services/dynamic-sidecar/src/simcore_service_dynamic_sidecar/modules/outputs/_manager.py
Show resolved
Hide resolved
|
@Mergifyio queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at 26bc00b |
|



What do these changes do?
Without altering the current pattern, this fix addresses the issue reported by sonarcloud.
Related issue/s
_uploading_task_startto avoid task being garbage collected and a more robust upload+clean task #8111How to test
Dev-ops