-
Notifications
You must be signed in to change notification settings - Fork 132
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
Collect stack frames immediately in Ruby 3.0 #150
Conversation
Because this is in the postponed job handler, we should never be able to reenter this code. I also believe this has a race condition if another signal arrives between the if statement and the increment. Signals can also arrive in any thread so this would not be a safe way to avoid reentrancy.
This entures we won't re-enter the signal handler from another signal whether or not it happens in our own thread.
Co-authored-by: Aaron Patterson <[email protected]>
I think being in a signal handler gives up on the the "golden rule of multithreading": "a single thread should see everything it does as if it was done in that order". AFAIK the signal could trigger anywhere, e.g. between two writes on |
+1 I planned experimenting with this and raising it at some point -- the change in ruby/ruby@0e276dc did reorder things as far as the C source goes, but as far as I see it there really doesn't seem to be anything guaranteeing that the compiler won't reorder the write to So... yeah this doesn't seem particularly safe at this moment. (But it would be great if |
As @eregon and @ivoanjo have said, I think there is a theoretical concern with this change. However, it may not lead to a visible error on major architectures and C compilers. Even in a potentially reproducible environment, it will be very unlikely to be visible unless the signal handler is called at an extremely unlucky timing. I think it is a possible option to ignore such a theoretical concern and take the practical benefits, but it's a difficult decision. I don't interfere with your decisions in stackprof, but if it were the interpreter itself, I would like to be very cautious. |
As of ruby/ruby@0e276dc, which shipped in Ruby 3.0, it seems to be safe to collect stack frames inside the signal handler. This should allow more accurate results than waiting for the postponed job to run since that can only measure when interrupts are checked.
This new behaviour is wrapped inside a preprocessor check for Ruby 3+
Additionally, this moves the "in signal handler" checks up a level, and uses pthread_mutex_trylock, which should be more reliable and I believe will fix the issue described in #123 and #124
@tenderlove and I had some discussion about whether this could have issues due to the writes in ruby/ruby@0e276dc being reordered.
One concern is whether there could be issues with hardware memory reordering (particularly on arm). I believe this is safe only because we are only considering the stack from our current thread. These writes will appear consistent in our interrupt handler because of this.
Another concern is that the compiler could reorder the writes in ruby/ruby@0e276dc. It doesn't seem to be, but that could absolutely happen. I think we should investigate making a change to Ruby to ensure the writes in
vm_push_frame
/vm_pop_frame
aren't reordered.