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

exit! incorrectly calls exit handlers #6379

rjattrill opened this issue Aug 31, 2020 · 6 comments

exit! incorrectly calls exit handlers #6379

rjattrill opened this issue Aug 31, 2020 · 6 comments


Copy link

Environment Information

  • jruby (2.5.7) 2020-08-03 9a89c94 OpenJDK 64-Bit Server VM 25.252-b09 on 1.8.0_252-b09 +jit [mswin32-x86_64]
  • Windows 10

Expected Behavior

Kernel::exit! should not fire exit handlers as per this documentation:

For example this script:

at_exit { puts "at_exit function" }
ObjectSpace.define_finalizer("string",  proc { puts "in finalizer" })

should produce no output and terminate. MRI behaves as expected.

Actual Behavior

The actual output is this:

at_exit function
in finalizer

Copy link

See for a discussion.

Copy link

chrisseaton commented Sep 2, 2020

I've read that Stack Overflow - but there's something missing here. There are specs for it

it "skips at_exit handlers" do
out = ruby_exe("at_exit { STDERR.puts 'at_exit' }; #{@object}.send(:exit!, 21)", args: '2>&1')
out.should == ""
$?.exitstatus.should == 21

but unlike what the Stack Overflow says, they don't seem to be tagged

fails:Kernel.at_exit both exceptions in at_exit and in the main script are printed

and the continuous-integration tests are passing. So there's something else going on.

Possibly, it is excluded so it isn't being run in the first place? I'd be surprised if they aren't running slow specs anywhere.

SPEC_DIR + '/core/kernel/at_exit_spec.rb',

Copy link

headius commented Sep 16, 2020

This appears to be an issue already fixed on master, probably due to reworks of the shutdown logic that have happened this year.

JRuby does indeed exhibit the behavior you reported:

$ rvm jruby- do ruby blah.rb
at_exit function
in finalizer

On master, the script behaves as it does in CRuby:

[] ~/projects/jruby $ cat blah.rb
at_exit { puts "at_exit function" }
ObjectSpace.define_finalizer("string",  proc { puts "in finalizer" })
[] ~/projects/jruby $ jruby -v blah.rb
jruby (2.6.5) 2020-09-16 57e3167d7b OpenJDK 64-Bit Server VM 25.222-b10 on 1.8.0_222-b10 +jit [darwin-x86_64]

[] ~/projects/jruby $ 

In addition, all relevant at_exit and Process.exit specs pass on JRuby master, except for the two shown below that are unrelated to this issue:

- is a private method
- runs after all other code
- runs in reverse order of registration
- allows calling exit inside at_exit handler
- gives access to the last raised exception
- both exceptions in at_exit and in the main script are printed (FAILED - 1)
- decides the exit status if both at_exit and the main script raise SystemExit
- runs all at_exit even if some raise exceptions
- runs at_exit handlers even if the main script fails to parse

- needs to be reviewed for spec completeness

- raises a SystemExit with status 0
- raises a SystemExit with the specified status
- raises a SystemExit with the specified boolean status
- tries to convert the passed argument to an Integer using #to_int
- converts the passed Float argument to an Integer
- raises TypeError if can't convert the argument to an Integer
- raises the SystemExit in the main thread if it reaches the top-level handler of another thread

- exits with the given status
- exits when called from a thread
- exits when called from a fiber
- skips at_exit handlers
- overrides the original exception and exit status when called from #at_exit (FAILED - 1)

I believe we can call this fixed for 9.3.

The remaining question is whether a fix should be provided on the 9.2 branch, in case we have additional 9.2.x maintenance releases before 9.3 is done (likely).

@rjattrill Did you run into this in an existing application or library, or did you just happen to notice the missing behavior? In short, what's the impact to you of us not releasing a fix until JRuby 9.3?

@headius headius added this to the JRuby milestone Sep 16, 2020
Copy link

rjattrill commented Sep 17, 2020

Thank you @headius for asking about the impact. The problem arose for us in an existing application that we have installed with customers. We are looking for a shortcut to close down our application. We have actors running in celluloid that we can't terminate easily. Our app has memory leaks in some contexts and so we are now cycling it manually.

Due to our production and QA cycles it would take us approximately 2 or 3 months to include a new release of JRuby in our product - so if 9.3.0 is just a few months away there is no point. However, assuming 9.3.0 is more than a few months away, then if it is not too hard to patch this into the next 9.2 maintenance release then that would be very much appreciated.

Either way - thank you for fixing this.

Copy link

headius commented Sep 17, 2020

I believe the PR that actually fixed this was #6212.

That PR was intended to reorder the teardown sequence to fix #6212 where we were shutting down the background thread pool used for Timeout before the rest of Ruby. This caused at_exit hooks to blow up if they attempted to use Timeout.

Along the way to fixing that, I noticed that we were installing a JVM shutdown hook to "make sure" we tear down the main JRuby runtime at JVM shutdown. This led to a deadlock in the new logic, since a hard exit! would try to clean up remaining threads, deadlocking on the same thread that initiated the exit! call. That hook was removed in c941c41, avoiding the "forced" teardown that I believe we are seeing here.

I believe we could fix this for 9.2.14 by just removing the shutdown hook. There's no good reason for us to force a teardown if someone requests a hard exit, and obviously doing so is causing us to run teardown logic when we should not.

@enebo What do you think about the impact to 9.2 behavior if we cherry-pick c941c41 over there for 9.2.14?

Copy link

headius commented Sep 17, 2020

I have created #6395 in case we decide the impact to 9.2 is ok.

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

No branches or pull requests

4 participants