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

RFC: Allow calls to exit from within exit hooks #32502

Merged
merged 1 commit into from
Nov 20, 2019
Merged

RFC: Allow calls to exit from within exit hooks #32502

merged 1 commit into from
Nov 20, 2019

Conversation

DilumAluthge
Copy link
Member

@DilumAluthge DilumAluthge commented Jul 5, 2019

Fixes #31743

This is an alternative to pull request #32253. I personally prefer the approach in #32253.

This pull request allows calls to exit(n) from within exit hooks registered with Base.atexit.

From the updated docstring for Base.atexit:

  • Exit hooks are allowed to call exit(n), in which case Julia will exit with
    exit code n (instead of the original exit code). If more than one exit hook
    calls exit(n), then Julia will exit with the exit code corresponding to the
    last called exit hook that calls exit(n). (Because exit hooks are called in
    LIFO order, "last called" is equivalent to "first registered".)

I have included some tests. They are located in the test/atexit.jl file.

@DilumAluthge DilumAluthge changed the title Allow calls to exit from within exit hooks RFC: Allow calls to exit from within exit hooks Jul 5, 2019
@DilumAluthge
Copy link
Member Author

One of the consequences of this pull request is that this code:

atexit(() -> exit(0))
atexit(() -> exit(5))
atexit(() -> exit(10))
exit(15)

will cause Julia to exit with exit code 0.

I'm not sure that this behavior makes sense. Is this what we want?

@DilumAluthge
Copy link
Member Author

Closing in favor of #32253

@DilumAluthge DilumAluthge deleted the da/allow-calls-to-exit-within-exithooks branch August 1, 2019 11:17
@vtjnash
Copy link
Sponsor Member

vtjnash commented Aug 1, 2019

FWIW, I think we should do both

@DilumAluthge DilumAluthge restored the da/allow-calls-to-exit-within-exithooks branch August 1, 2019 17:35
@DilumAluthge DilumAluthge reopened this Aug 1, 2019
@DilumAluthge
Copy link
Member Author

Reopened!

test/atexit.jl Outdated Show resolved Hide resolved
Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems like a lot of tests here. How long do these take? It doesn't seem like it should need that many to fully cover the cases.

@JeffBezanson
Copy link
Sponsor Member

I like this change; it's very elegant. For reference, posix seems to allow atexit handlers to call _exit(n), which causes the process to exit immediately without running further handlers. Calling exit is undefined. So we are doing something else, but that might be ok.

@DilumAluthge
Copy link
Member Author

There seems like a lot of tests here. How long do these take? It doesn't seem like it should need that many to fully cover the cases.

I've deleted a bunch of tests. There are relatively few tests now, but I think we still cover all the cases.

@DilumAluthge
Copy link
Member Author

Bump @vtjnash Take a look and let me know if I’ve addressed your concerns with regards to the number of tests.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Nov 18, 2019

Yep. Could I ask you to rebase and push an updated copy? We had some CI issues on this run (from stuff on master at the time), and I'd like to check that it's green before merging.

@DilumAluthge
Copy link
Member Author

Could I ask you to rebase and push an updated copy?

Done!

@vtjnash vtjnash merged commit 45721be into JuliaLang:master Nov 20, 2019
@DilumAluthge DilumAluthge deleted the da/allow-calls-to-exit-within-exithooks branch November 20, 2019 17:31
""" => 1,
# ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
"""
atexit(() -> println("No error"))
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind making a quick followup PR to make these silent during a normal test run?

Suggested change
atexit(() -> println("No error"))
atexit(() -> "No error")

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah!

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

Successfully merging this pull request may close these issues.

Feature request: Allow exit hooks (registered with atexit) to change the exit code
3 participants