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

Properly close inherited servers on fork #93

Merged
merged 1 commit into from
Sep 25, 2023
Merged

Conversation

casperisfine
Copy link
Contributor

I noticed some UNIXServer and AppServer accumulating with reforking enabled.

The reason is that the at_exit { stop } make Server instances immortal, so they are never GC, and their open socket accumulate and are never closed.

That callback is useless anyway, as on exit the finalizer of these objects will be invoked, and they'll cleanup after themselves.

@casperisfine
Copy link
Contributor Author

That callback is useless anyway, as on exit the finalizer of these objects will be invoked, and they'll cleanup after themselves.

My bad on that one. UNIXServer doesn't delete the file. So I replaced the at_exit by an object finalizer.

I noticed some UNIXServer and AppServer accumulating with
reforking enabled.

The reason is that the `at_exit { stop }` make `Server` instances
immortal, so they are never GC, and their open socket accumulate and
are never closed.

That callback is useless anyway, as on exit the finalizer of these
objects will be invoked, and they'll cleanup after themselves.
Copy link
Member

@dalehamel dalehamel left a comment

Choose a reason for hiding this comment

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

❤️ thanks for this, and sorry for this oversight.

@casperisfine
Copy link
Contributor Author

sorry for this oversight.

No worries, it's a common footgun.

@casperisfine casperisfine merged commit 0df2b64 into main Sep 25, 2023
6 checks passed
@shopify-shipit shopify-shipit bot temporarily deployed to production September 25, 2023 12:25 Inactive
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