[Bug] ensure clause not run when call `exit`early

Following code only output “boom!”

# 1.cr

begin
  raise "boom!"
rescue e : Exception
  puts e.message
  exit
ensure
  puts 1
end

But same code in Ruby will ensure code in ensure clause will always be run.


# 1.rb
begin
  raise "boom!"
rescue Exception => e
  puts e.message
  exit
ensure
  puts 1
end

# boom!
# 1

So, this is a Crystal bug, right?

1 Like

Well, depends on how you look at it. Apparently ensure isn’t run when you exit. The docs says:

An ensure clause is executed at the end of a begin ... end or begin ... rescue ... end expression regardless of whether an exception was raised or not:

Thing is, the exit means that the end of the begin is never reached as such, as the program is, well, stopped.

It works in Ruby because exit in Ruby raises SystemExit, which means we’re still running, while Crystal exit Terminates execution immediately, returning the given status code to the invoking environment.

One could argue that Rubys usage of SystemExit is a bit odd, as exiting isn’t an error condition but part of normal program flow. But I kinda liked it.

7 Likes

Related, there is the explicit at_exit handler if you think there is some cleanup that must happen. It could be nice to define an ensure block that could automatically be added as an at_exit hook, just to make sure the ensure runs at least once (either during normal program flow or when the program exits prematurely).

1 Like

I was just past one of my earlier apps, where I define an Exit and an Abort exception and throw them to signal exit. The only call to exit is in the main file, which catches those exceptions.

Yes, i consider those things shouldn’t be handle by user, it should handle by std-lib, just like Ruby.

No core team member to give some advice?

Xen explained it well: the documentation is clear as to what to expect w.r.t. exit, and I add that being able to simply pull the plug is a good thing. Otherwise, you can always raise an exception, as also Xen mentioned.

An alternative implementation could be to have Error like in Java: something that is not meant to be rescued (unless explicitly mentioned). This has the advantage that you could provide all the nice things about custom-made exceptions, plus all ensures being ensured, and at the the end the at_exit handler being called. And it won’t be catched by catch-all clauses that do not explicitly mention the Error.

3 Likes

Perhaps how this behavior should be worth a poll.

I can’t think of any reasonable application for this. Calling exit is usually a part of the top level program interface and there I wouldn’t expect any relevant cleanup. In deeper paths the most likely reason to call exit directly is because the program has reached some state where continuation is impossible (or unwanted) for some reason.
Could anyone share some practical use case where you want a process to exit and also wrap up loose ends in ensure blocks?

2 Likes

Check this commit for fix this pitfall (at least, as a Rubyist, i thought it should work), sure i eliminate the need of call exit after this commit, but, i guess many user from Ruby thought original code should work.

Following is above code reduced version.

before commit,ensure not work when exit.

begin
  chan = Channel(Tuple(String, String, Time::Span)).new

  driver, capabilities = create_driver(browser, debug_mode)

  begin
    start_time = Time.monotonic

    if engine_list.includes? Engine::Youdao
      spawn Youdao.new(create_session(driver, capabilities), content, debug_mode, chan, start_time)
    end

    if engine_list.includes? Engine::Tencent
      spawn Tencent.new(create_session(driver, capabilities), content, debug_mode, chan, start_time)
    end
  rescue e : Selenium::Error
    e.inspect_with_backtrace(STDERR)
    exit
  end

  DB.open DB_FILE do |db|
    engine_list.size.times do
      select
      when result = chan.receive
	    # write DB
        puts "Success"
      when timeout timeout_seconds.seconds
        STDERR.puts "Timeout!"
      end
    end
  end
ensure
  sleep 0.05
  driver.stop if driver
end

Perhaps those code not good, need refactor, but, it should behavior correct.
when raise Selenium::Error, i want to skip write DB, but ensure stop driver.

new version (still not good, but behavior correct)

begin
  chan = Channel(Tuple(String, String, Time::Span)).new

  driver, capabilities = create_driver(browser, debug_mode)

  start_time = Time.monotonic

  if engine_list.includes? Engine::Youdao
    spawn Youdao.new(create_session(driver, capabilities), content, debug_mode, chan, start_time)
  end

  if engine_list.includes? Engine::Tencent
    spawn Tencent.new(create_session(driver, capabilities), content, debug_mode, chan, start_time)
  end

  DB.open DB_FILE do |db|
    engine_list.size.times do
      select
      when result = chan.receive
        # save db
        puts "Success"
      when timeout timeout_seconds.seconds
        STDERR.puts "Timeout!"
      end
    end
  end
rescue e : Selenium::Error
  e.inspect_with_backtrace(STDERR)
ensure
  sleep 0.05
  driver.stop if driver
end

As you can see, i have to wrap the DB save logic into the begin/rescue block,
to ensure driver always stop when raise Selenium::Error, but that really not necessary,
we should always make the begin/rescue block as small as possible
(only include the code which can raise Selenium::Error), right?


I don’t know if this is a good example, anyway, this is a issue i meet when i coding.

Well, then you should simply not call exit until you actually mean it?
This comes down to the fact that Crystal just is not Ruby.
It has different syntax and semantics in many places, and yes, it does have “false friends” where the same word in crystal does not mean the same thing as in ruby.
I think maybe this could be made more prominent in the documentation.

Crystal is not Ruby. It just provides similar joy when using it.

This also reveals another shortage of Crystal, because it missing the control structure like catch/throw in Ruby, so, have to use ensure to skip some code structure, but, exit limit the usage of ensure.

In this case, I believe that re-raising the exception instead of calling exit would get you what you want?

begin
    # ...
rescue e : Selenium::Error
    e.inspect_with_backtrace(STDERR)
    raise e
end
1 Like

Not same, re-raising exception will always result in back trace and return nonzero, but what i want just print somethings.

Even we are not consider this different, we are use the dirty hack to fix another error, you really should avoid re-raising exception in rescue, that will result in wired back trace, hard to debug.

I think this is a good idea, as it can trip up Ruby people.

At first, I was “couldn’t Crystal be more like Ruby” on this one, but after letting it brew a bit, I kinda like it. exit means exit, it’s the big “stop everything!” button, which should be approached with appropiate caution. It is a bit oddish that exit in Ruby is actually a shorthand for raise. Always thought that, even though I found it bloody useful. Crystals behavior also means that we don’t need exit!

Making my own PleaseExit exception classes works for m.

3 Likes

The only drawback of PleaseExit is that it’ll get caught by generic Exception rescues. You need to have strong discipline in your code to not fall for it.

Well, yeah, there is that. But I don’t know about “strong” discipline. I tend to use generic rescues on not-my-own code, and they don’t throw PleaseExit. Around my own code, I tend to be more explicit in what I rescue. Not that there’s not edge-cases where there might be issues, but I don’t feel it’s much of a problem in general.

1 Like