-
Notifications
You must be signed in to change notification settings - Fork 0
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: race condition when initializing multiprocessing manager #26
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #26 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 9 9
Lines 334 337 +3
=========================================
+ Hits 334 337 +3 ☔ View full report in Codecov by Sentry. |
🦙 MegaLinter status: ✅ SUCCESS
See detailed report in MegaLinter reports |
…DS/Runner into python-3.12-linux-deadlock # Conflicts: # src/safeds_runner/server/pipeline_manager.py
The process seems to get stuck inside |
That's true, but this does not seem to be the (main) problem. The main process gets stuck there because it doesn't receive any messages from the pipeline process. This seems to be related to a race condition when initializing the message queue. Note:
This caused the multiprocessing manager to sometimes be initialized twice (instead of only once), and the main process would get a different queue than the pipeline process. Because no messages were sent to the correct queue, the main process waited indefinitely for results. I confirmed this working under WSL2 with Python 3.12 (or I was incredibly lucky, and no problem has happened since I made the change). So in the end, there was no deadlock (according to these debugging results), and the fork deprecation message turned out to be a red herring. Although, forking in a threaded program is still bad practice and should be avoided (and is also removed in this PR). |
fix: more strict locking during mocking tests
@SmiteDeluxe This should also fix the issue, that the extension sometimes isn't notified of the current pipeline execution progress |
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.
I can no longer reproduce #18. Thanks for the fix.
🎉 This PR is included in version 0.4.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Closes #18
Summary of Changes