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

Possible bug with Ruby >= 2.7.0 and `GC.compact` #241

Closed
boazsegev opened this issue Aug 30, 2020 · 3 comments
Closed

Possible bug with Ruby >= 2.7.0 and `GC.compact` #241

boazsegev opened this issue Aug 30, 2020 · 3 comments
Milestone

Comments

@boazsegev
Copy link
Contributor

@boazsegev boazsegev commented Aug 30, 2020

The issue was discovered by @palkan and discussed here (anycable/simple-cable-app#6)

The new feature that helps with heap fragmentation, GC.compact moves Ruby objects in the heap, changing their pointers.

Some objects are considered immovable, such as C classes and (perhaps) their instances. Also, any object referenced by a C extension using the rb_gc_mark callback is considered immovable.

nio4r uses an Array container to store references here. This array is marked with rb_gc_mark which makes it a safe, immovable, array.

However, the Ruby GC is free to move the objects within the array to a new location (while updating the reference within the Ruby array).

Only C objects are immovable, but Ruby IO objects can be moved around since they aren't independently marked by rb_gc_mark.

This means that whenever nio4r collects a reference to it's IO data structure (i.e., here and here), it is collecting a pointer that may have been moved (or some of the data within the data structure may point to an invalid pointer that was already moved).

I understand that creating an all-object GC mark data structure can be an expensive task, but there are many possible C implementations of the Set data structure, such as the one I implemented in iodine (extracted to the facio.io C STL repo and documented here.

All the code I pointed to for iodine and facil.io is MIT licensed. You're welcome to use it.

Let me know if I can help.

@ioquatix
Copy link
Member

@ioquatix ioquatix commented Aug 31, 2020

Do you have time to make a PR? I will accept it. My time right now is very limited.

@boazsegev
Copy link
Contributor Author

@boazsegev boazsegev commented Aug 31, 2020

I think it might take me a week or two, since:

  1. I've never used nioi4r and I don't know the API that well.

  2. I'm a 42 year old student (again), dealing with both normal life deadlines and the end of my online semester (final projects) 😅

But I can play around and see what I can do... if the timeframe is fine.

This was referenced Aug 31, 2020
ioquatix added a commit that referenced this issue Sep 5, 2020
This should fix the dangling pointer issue caused by `GC.compact` moving the monitor object's memory address to a new location.
@ioquatix ioquatix added this to the v2.5.3 milestone Sep 7, 2020
@ioquatix
Copy link
Member

@ioquatix ioquatix commented Sep 7, 2020

This fix has been merged and will be released shortly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.