Fix exit in at_exit handlers

[bew]

RFC!

There's no specs for at_exit stuff. Currently I made some basic test files to test the new behavior, I'll put them here in the description, along with their output, so we can discuss this new behavior, and if it's a good idea (or not).
I'll add them as specs later if needed.

Test: exit in a at_exit handler after a successful process execution changes the process's final status code

at_exit do |status|
  puts "1st at_exit handler, status is #{status}"
end

at_exit do |status|
  puts "2nd at_exit handler, status is #{status}"
  puts "Exiting with 2..."
  exit 2
end

at_exit do |status|
  puts "3rd at_exit handler, status is #{status}"
end

Exits with status 2, outputs:

3rd at_exit handler, status is 0
2nd at_exit handler, status is 0
Exiting with 2...
1st at_exit handler, status is 2

Test: exit in a at_exit handler after an explicit exit call changes the process's final status code

at_exit do |status|
  puts "1st at_exit handler, status is #{status}"
end

at_exit do |status|
  puts "2nd at_exit handler, status is #{status}"
  puts "Exiting with 2..."
  exit 2
end

at_exit do |status|
  puts "3rd at_exit handler, status is #{status}"
end

exit 1

Exits with status 2, outputs:

3rd at_exit handler, status is 1
2nd at_exit handler, status is 1
Exiting with 2...
1st at_exit handler, status is 2

Test: raise in a at_exit handler changes the status code if it was not already > 1

at_exit do |status|
  puts "1st at_exit handler, status is #{status}"
end

at_exit do |status|
  puts "2nd at_exit handler, status is #{status}"
  raise "Raised from at_exit !!"
end

at_exit do |status|
  puts "3rd at_exit handler, status is #{status}"
end

Exits with status 1, outputs:

3rd at_exit handler, status is 0
2nd at_exit handler, status is 0
Error running at_exit handler: Raised from at_exit !!
1st at_exit handler, status is 1

Fixes #5409 (comment)

[larubujo]

last status should win, dont think max makes sense. ruby not work this way.

[bew]

Yeah I'm not sure about this too, was just a try anyway!
My idea was: if the handler has an exit (or exit 0) and the process is in-the-process-of-exiting with a exit 1 (or more), then it should not exit with a status code 0 but with 1.

No idea how Ruby works about that.

[larubujo]

ruby seem to use status code from exit that was last executed. each exit changes status code

[RX14]

Indeed, please use the latest exit code.

[straight-shoota]

Great, this prevents the error from #5409 wrongly reporting as exit status 0 in the circle CI build. So failed CI is actually confirming this change. =)

[RX14]

it shouldn't be possible to rescue a nested exit, and it shouldn't use stack unwinding. Please make calling exit simply resumes where the previous exit call left off. Probably just by removing at_exit handlers when they are run.

[RX14]

Surely there would be a lot less duplication if this main function simply called exit with the status and never returned.

[bew]

Good idea, I'll do that

[bew]

But this will make main never return, that's probably not what we want IMO, it'll probably break some other things if we do that.

@ysbaddaden do you know something about this?
I'd like to have input from @asterite too (let's hope 😃)

[bew]

Comment #5413 (comment) extracted:

Context: main should just call exit status to reduce code duplication

But this will make main never return, that's probably not what we want IMO, it'll probably break some other things if we do that.

@ysbaddaden do you know something about this?
I'd like to have input from @asterite too (let's hope 😃)

[RX14]

@bew why would that break anything?

[RX14]

The only difference is between returning from main and calling LibC.exit. And libc does that anyway (yes, returning from main just means _init will call exit() for you: see the glibc source)

[bew]

Hmm probably. I was thinking that it may break programs that redefine main, but I haven't put much thought about it actually.

[RX14]

Hmm, actually it probably will break those programs. My bad, I didn't think of that usecase.

Perhaps we should have a :nodoc: function shared between the two methods. Or perhaps we could define all the other items as at_exit handlers so exit() simply called AtExitHandlers then LibC.exit.

[bew]

we could define all the other items as at_exit handlers so exit() simply called AtExitHandlers then LibC.exit

That's also something I was thinking, done!

[bcardiff]

I am not totally convinced that handling the flush and restore of blocking state should be done with at_exit. I would rather leave some explicit calls for this. at_exit can't be cleared now, but might in the future. That way, also @@handlers would remain as nil in most of the cases.

[bew]

Maybe we should have multiple kind of at exit handlers? Like the app's at_exit handlers with the user's code (which can be cleared), and the crystal's at_exit handlers?

[bew]

nvm, it's just easier/faster to just duplicate the flushing+restore blocking, it's really simple code.

(was thinking of putting this in a class method in Crystal namespace (in src/crystal/main.cr) but I can't find a good name for it..)

[bew]

@bcardiff I rebased, and reverted the use of at_exit to recover stdin/out blocking state.

I also added the stdin/ou blocking state recovery to exit when it's called explicitly (not only when the program ends by finishing the main).

[RX14]

Actually, please write a note to explain we assign to @@handler_iterator to make sure we only call each handler once, even if an AtExitHandler calls exit().

[bew]

Good idea, done!

[bew]

@RX14 I added the note about @@handler_iterator you requested, should be GTG!

I was thinking about adding specs, but I don't know how this can be properly spec'ed.. I think I would need to compile a snippet using the new stdlib, then execute it and check the output & status code. But it doesn't sound right for a 'spec'. WDYT?

[RX14]

Actually I think we already have specs which invoke the crystal compiler through bin/crystal.

[bew]

I rebased/squashed, and added some specs for exit & at_exit, using bin/crystal for now to compile the necessary snippets.
I took inspiration in spec/std/callstack_spec.cr@master for the part using bin/crystal and temporary files.

Let me know what you think!

[bew]

Can someone restart the Travis build? It failed due to a fork: Cannot allocate memory error, on an unrelated spec.

[asterite]

It seems this changes this: in case of an unhandled exception, the exception gets printed before the exit handlers. At least in Ruby it works the other way around, and the old code works like that. Please change it back.

[bew]

@asterite why is it important to print the exception after the exit handlers?

[asterite]

One reason is that if you have an error in your program and you have an exit handler print stuff, the error might be hidden after all those messages.

Another way to think about this is that printing the error is the first exit handler, and other exit handlers are added next. Then, exit handlers run in reverse order, so printing the exception should come last.

The final reason is that Ruby does it this way, so there must be a good reason.

[RX14]

This should probably raise while we're iterating (i.e. @@handler_iterator != nil) to stop people modifying the exit handlers in an exit handler.

[asterite]

I just tried it in Ruby, it works. We just need to do a loop with an index (in case it doesn't work already).

[asterite]

I mean, this works in Ruby:

at_exit do
  at_exit do
    puts "BYE"
  end
  puts "EXIT"
end

Ouptut:

EXIT
BYE

[RX14]

I don't think it should work. It's confusing and creates for actively confusing code. Let's not follow ruby's example on this.

[bew]

What would be the usecase of having at exit handler adding new at exit handlers? @asterite

[bew]

For the record, in ruby the nested at_exit handlers are run just after the handler around them:

at_exit do
  puts "first"
end

at_exit do
  at_exit do
    puts "> nested"
  end
  puts "second"
end

at_exit do
  puts "third"
end

Gives:

third
second
> nested
first

---

Not sure it can be useful for anything with this behavior...

[asterite]

Yes, at_exit adds a handler to the end of the list, and to traverse them we should use pop?. Super simple. Right now it uses reverse_each, but a change in my local copy to use pop? made it work.

[RX14]

I agree that this is how it should work if we want this to work.

However I don't see the usecase, and so I'd rather be more restrictive until we knew what the usecases were. This is because it creates a far more complex and dynamic control flow through the handlers which may be hard to track and hard to debug. So i'd err on the side of saying "no".

[asterite]

Sounds good. We can wait until someone complaints with a real use case.

[RX14]

That being said, the pop? code is robust whether we disallow this or not, and is smaller and easier to read. If we decide to enable it it should be as easy as removing a raise ... if line of code.

[bew]

I see.. One issue which can arise is in the case where an exit handler calls exit, the execution won't come back to main and the exception might never be printed.

I think that a way to ensure the exception is properly printed even in case of exit in handler, would be to insert an exit handler (at position 0) just before running the handlers, that would print the exception (like the thinking mode suggestion from @asterite).

[bew]

I refactored the handler loop with pop?, I agree, it's simpler and better! thanks @asterite
I also disabled nested at_exit handlers and added a spec for it @RX14.

Did you see my last comment ?

[bew]

I have this waiting in my fork, should I include it in this PR?
bew/crystal@fix-exit-in-at_exit-handlers...print-full-stacktrace-of-at_exit-errors

It just print the full stacktrace when an exception occurs in a at_exit handler (instead of just the message)

[bew]

Anything left to do?

[RX14]

When would you have more than one?

[bew]

Currently never, I was just thinking that maybe some shards might want to add multiple exceptions.. But now that I rethink about it it doesn't make sense, or at least I shoukd apply YAGNI until someone needs this.

[RX14]

"in an" not "from an"

[bew]

Note for myself: don't forget to re-add this line

[bew]

I've rebased, and applied @RX14's comments, should be GTG I think when CI passes!

Fixed

[bew]

Cool thank you!