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

Fix a bug causing pipe FDs to leak #74

Merged
merged 1 commit into from
Feb 27, 2023

Conversation

jealouscloud
Copy link
Contributor

@jealouscloud jealouscloud commented Apr 14, 2022

This addresses issue #51

multiprocessing.Process instances hold pipes open until their .close() method has been called, even if the child process has exited.

Manually grabbing the data from them on exit and then closing the multiprocess.Process prevents this leak.

@@ -157,9 +158,6 @@ def _advance_the_queue():
if self._debug:
print("Job queue finished.")

for job in self._completed:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

join() here blocks until the process has ended, but if it's in _completed it already ended. This was unnecessary.

@ploxiln
Copy link
Owner

ploxiln commented Apr 15, 2022

Tests seem to fail for python-3.6 and earlier?

   File "/__w/fab-classic/fab-classic/tests/mock_streams.py", line 74, in inner_wrapper
    func(*args, **kwargs)
  File "/__w/fab-classic/fab-classic/tests/test_network.py", line 607, in test_should_abort_when_cannot_connect
    execute(parallel_subtask, hosts=['nope.nonexistent.com'])
  File "/__w/fab-classic/fab-classic/fabric/tasks.py", line 383, in execute
    ran_jobs = jobs.run()
  File "/__w/fab-classic/fab-classic/fabric/job_queue.py", line 152, in run
    done.close()
AttributeError: 'Process' object has no attribute 'close'

@jealouscloud
Copy link
Contributor Author

I tested del which seems to close the related FD as well. Added checks for multiprocessing.Process.close, which will be called on Python version that support it.

@Stealthii
Copy link

@jealouscloud this compatibility change for Python 3.6 seems to work, great work. Although out of support it's still the default version provided by EL7 capable distros, and only Python<3.8 works with fab-classic's current parallel implementation.

@ploxiln
Copy link
Owner

ploxiln commented Feb 27, 2023

I am sorry I neglected this for nearly a year. Looks good - I am just going to rebase and squash before merge ...

need to close() or unref the multiprocessing.Process
@ploxiln ploxiln merged commit 8e61a2b into ploxiln:master Feb 27, 2023
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.

3 participants