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

[Experimental] Pitchfork::Info.close_all_fds! #56

Merged
merged 2 commits into from
Aug 2, 2023
Merged

Conversation

casperisfine
Copy link
Contributor

Ensuring that an application is fork safe can be complex, and in case of mistake, one of the error condition is shared file descriptors, causing one worker to read packets from another which could have dire consequences.

Rather than to track down all the file descriptors that need reopening, close_all_fds! allows to automatically close all file descriptors that aren't explicitly marked as needing to be kept.

And in case of mistake, the error condition is that trying to read or write into that file descriptor will fail, which is much less critical than to read packets from another worker.

@casperisfine
Copy link
Contributor Author

And in case of mistake, the error condition is that trying to read or write into that file descriptor will fail, which is much less critical than to read packets from another worker.

Correction here. FDs are fairly heavily recycled, so if we close the FD of some native code without its knowledge, while it's very likely it will just crash, it could end up reading or writing in a random file or socket, so it could potentially be worse.

In the end I think this approach make sense, but should be limited to Ruby level IOs via ObjectSpace.each_object(IO, &:close) given that Ruby IOs automatically keep track of them being closed and won't attempt to read/write the FD anymore.

That should take care of any potential fork safety issue in pure Ruby gems, but doesn't solve potential issues in native gems.

One downside is that ObjectSpace.each_object is quite slow on large heaps.

Ensuring that an application is fork safe can be complex, and
in case of mistake, one of the error condition is shared file
descriptors, causing one worker to read packets from another
which could have dire consequences.

Rather than to track down all the file descriptors that need
reopening, `close_all_fds!` allows to automatically close all
file descriptors that aren't explicitly marked as needing to
be kept.

And in case of mistake, the error condition is that trying to
read or write into that file descriptor will fail, which is
much less critical than to read packets from another worker.
@casperisfine casperisfine force-pushed the close-fds branch 4 times, most recently from 188fcf8 to 4268930 Compare August 2, 2023 14:59
@casperisfine casperisfine marked this pull request as ready for review August 2, 2023 15:04
@casperisfine casperisfine merged commit 0290e18 into master Aug 2, 2023
20 of 21 checks passed
casperisfine pushed a commit to Shopify/ci-queue that referenced this pull request Aug 2, 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.

2 participants