Skip to content
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

Test macOS #92

Merged
merged 9 commits into from Oct 2, 2023
Merged

Test macOS #92

merged 9 commits into from Oct 2, 2023

Conversation

jonashaag
Copy link
Contributor

No description provided.

@jonashaag
Copy link
Contributor Author

The gc.collect() calls seem to the fix the semaphore leakage but it feels kind of hacky

@jonashaag jonashaag marked this pull request as ready for review September 18, 2023 11:43
@sybrenjansen
Copy link
Owner

Until I have a mac I cannot test this, unfortunately. I will try to set up a VM in the near future, but I've been quite busy 😅

I can live with the hacky gc.collect() for now, but perhaps add a TODO in the code that this is not a proper fix. Additionally, let's remove the macos testing on github as other stuff probably won't run successfully yet.

This PR will at least fix a few things already for mac. Probably enough for people to get started. And when I get time to test it on mac we can add macos properly.

@@ -15,7 +15,7 @@ jobs:
runs-on: ${{ matrix.os }}
strategy:
matrix:
os: [ubuntu-20.04, windows-latest]
os: [ubuntu-20.04, windows-latest, macos-11]
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this for now, until everything is properly tested

mpire/comms.py Outdated
@@ -635,6 +639,8 @@ def drain_results_queue_terminate_worker(self, dont_wait_event: threading.Event)
except (queue.Empty, OSError):
if got_results:
dont_wait_event.set()
# Force collection of semaphore objects
gc.collect()
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add TODO that this still needs a proper fix, same for the other gc.collect

Copy link
Owner

@sybrenjansen sybrenjansen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor things left. Can you add a line to docs/changelog.rst with the changes you made? E.g., added a workaround for semaphore leakage on macOs + fixed a bug when working in a fork context while the system default is spawn.

You can put it under the unreleased section.

@sybrenjansen sybrenjansen merged commit d873e5a into sybrenjansen:master Oct 2, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants