-
-
Notifications
You must be signed in to change notification settings - Fork 85
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 for #241 #244
fix for #241 #244
Conversation
@tenderlove can you please comment on this? Thanks. |
This will indeed prevent the pointer at |
@tenderlove when monitoring for IO readiness (e.g. poll/epoll/kqueue), it registers a pointer to a C struct |
Thanks everyone for your feedback, it looks like this should solve the problems we are facing. Thanks for @boazsegev for figuring it out, for this specific problem I didn't even know where to start. |
Hey folks! It seems that this PR introduced a regression on Ruby 2.6.6. When upgrading from 2.5.2 to 2.5.3, we're experiencing segfaults on our CIs. Here's one of the backtraces we got:
Another one in the same isolated nio4r upgrade :
My knowledge on C libraries is limited and I don't know if the above backtraces is useful at all. |
That's a NULL pointer exception, although the location it's occurring isn't helpful: https://github.com/vcr/vcr/blob/v3.0.3/lib/vcr/structs.rb#L123 It may be a sign of memory corruption. |
@tarcieri thanks for looking into my comment, much appreciated! I was also thinking about memory corruption. The reason I ended up in this PR is that I can reproduce segfaults (like the one I posted above to this MR). The failure rate with this PR merged on our CI increased with 8% and all issues are related to segfaults. Our stack is huge so it's hard to pinpoint exactly what's going on. I'll try to create a dummy project to get this in a reproducible state but I can't promise any success. |
#251 fixed the issue, thanks! |
This should fix the dangling pointer issue caused by
GC.compact
(#241) moving the monitor object's memory address to a new location.