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: handle the case where GC runs during fork safety checks #592

Merged
merged 1 commit into from
Dec 8, 2024

Conversation

flavorjones
Copy link
Member

@flavorjones flavorjones commented Dec 4, 2024

If GC runs between the check for db.weakref_alive? and db.close then an exception will be raised:

Invalid Reference - probably recycled (WeakRef::RefError)

I personally saw this error when running a Rails app test suite in parallel, so while it's rare it does happen.

In this case, let's just swallow the error and keep going, since the database object isn't in use and was GCed.

If GC runs between the check for `db.weakref_alive?` and `db.close`
then an exception will be raised:

    Invalid Reference - probably recycled (WeakRef::RefError)

In this case, let's just swallow the error and keep going, since the
database object isn't in use and was GCed.
@flavorjones
Copy link
Member Author

@tenderlove or @byroot can I ask for a quick review of this?

I considered using ObjectSpace::WeakMap because its foreach has a GC guard on both key and value; but it doesn't have a #clear method so it didn't feel any cleaner than this PR.

@byroot
Copy link
Contributor

byroot commented Dec 4, 2024

Yeah, the missing #clear is a bit annoying but you can always replace the map entirely (but doesn't allow to store it in constant).

Copy link
Contributor

@byroot byroot left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@flavorjones flavorjones merged commit d774206 into main Dec 8, 2024
105 checks passed
@flavorjones flavorjones deleted the flavorjones-handle-weakref-cleanup branch December 8, 2024 18:34
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