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.

Sign up for GitHub

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

Jump to bottom

Should atexit_hooks be guarded during _atexit? #49841

Closed
NHDaly opened this issue May 16, 2023 · 2 comments · Fixed by #49868
Closed

Should atexit_hooks be guarded during _atexit? #49841

NHDaly opened this issue May 16, 2023 · 2 comments · Fixed by #49868
Labels
domain:multithreading Base.Threads and related functionality

Comments

@NHDaly
Copy link
Member

NHDaly commented May 16, 2023

Two questions, following up from #49774:

  1. Is it safe to grab a mutex inside _atexit()? @kpamnany and I were worried about the system being in an undefined state during shutdown, and we aren't sure if we're allowed to grab a mutex. My hope/guess is yes it's fine? But want to confirm.
  2. Is it possible for user tasks to still be scheduled during _atexit()?
    • If the user tasks can still be running, they might be adding new atexit_hooks while we are popping them and running them, which would be a data race.
    • If that's possible, we need to @lock atexit_hooks_lock inside _atexit().
    • My preference would be to lock it, either way, just to be safe, assuming 1. is resolved.

Questions best answered by: @vtjnash, @vchuravy, @gbaraldi?
@NHDaly NHDaly added the domain:multithreading Base.Threads and related functionality label May 16, 2023
@NHDaly NHDaly mentioned this issue May 16, 2023
Merged
@vtjnash
Copy link
Sponsor Member

vtjnash commented May 16, 2023

Yes, we run these callback hooks quite early, so they can block exit and threads are still chugging away

@NHDaly
Copy link
Member Author

NHDaly commented May 17, 2023

Okay thanks! Then we need to grab a lock. 👍 I'll put up one more PR

NHDaly added a commit that referenced this issue May 17, 2023
@NHDaly
Lock the atexit_hooks during execution of the hooks on shutdown.
34a8e44 
Fixes #49841.
@NHDaly NHDaly mentioned this issue May 17, 2023
Merged
vtjnash pushed a commit that referenced this issue May 30, 2023
@NHDaly
thread safety: lock around atexit_hooks global (#49868)
20752db 
Ensure the lock is precise, so that we are allowed to register new
atexit hooks from inside an atexit hook. But then disable `atexit()`
when shutting down after it finishes running.

Add tests that cover all the cases:
1. registering a hook from inside a hook
2. registering a hook from another thread while hooks are running
3. attempting to register a hook after all hooks have finished
   (disallowed)

Fixes #49841
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
NHDaly added a commit to RelationalAI/julia that referenced this issue Jun 7, 2023
@NHDaly
Lock the atexit_hooks during execution of the hooks on shutdown.
65c71ce 
Fixes JuliaLang#49841.

Follow-up to JuliaLang#49774.

This PR makes two changes:
1. It locks `atexit_hooks` while iterating the hooks during execution of `_atexit()` at shutdown.
    - This prevents any data races if another Task is registering a new atexit hook while the hooks are being evaluated.
2. It defines semantics for what happens if another Task attempts to register another atexit hook _after all the hooks have finished_, and we've proceeded on to the rest of shutdown.
    - Previously, those atexit hooks would be _ignored,_ which violates the user's expectations and violates the "atexit" contract.
    - Now, the attempt to register the atexit hook will **throw an exception,** which ensures that we never break our promise, since the user was never able to register the atexit hook at all.
    - This does mean that users will need to handle the thrown exception and likely do now whatever tear down they were hoping to delay until exit.
KristofferC pushed a commit that referenced this issue Jun 26, 2023
@NHDaly @KristofferC
thread safety: lock around atexit_hooks global (#49868)
f12d9cd 
Ensure the lock is precise, so that we are allowed to register new
atexit hooks from inside an atexit hook. But then disable `atexit()`
when shutting down after it finishes running.

Add tests that cover all the cases:
1. registering a hook from inside a hook
2. registering a hook from another thread while hooks are running
3. attempting to register a hook after all hooks have finished
   (disallowed)

Fixes #49841
Co-authored-by: Jameson Nash <vtjnash@gmail.com>

(cherry picked from commit 20752db)
NHDaly added a commit that referenced this issue Jul 19, 2023
@NHDaly
Lock the atexit_hooks during execution of the hooks on shutdown.
54ae3b6 
Fixes #49841.

Follow-up to #49774.

This PR makes two changes:
1. It locks `atexit_hooks` while iterating the hooks during execution of `_atexit()` at shutdown.
    - This prevents any data races if another Task is registering a new atexit hook while the hooks are being evaluated.
2. It defines semantics for what happens if another Task attempts to register another atexit hook _after all the hooks have finished_, and we've proceeded on to the rest of shutdown.
    - Previously, those atexit hooks would be _ignored,_ which violates the user's expectations and violates the "atexit" contract.
    - Now, the attempt to register the atexit hook will **throw an exception,** which ensures that we never break our promise, since the user was never able to register the atexit hook at all.
    - This does mean that users will need to handle the thrown exception and likely do now whatever tear down they were hoping to delay until exit.
NHDaly added a commit to RelationalAI/julia that referenced this issue Jul 21, 2023
@NHDaly
Backport 49868 (#20)
ddcbdb9 
* Lock the atexit_hooks during execution of the hooks on shutdown.

Fixes JuliaLang#49841.

Follow-up to JuliaLang#49774.

This PR makes two changes:
1. It locks `atexit_hooks` while iterating the hooks during execution of `_atexit()` at shutdown.
    - This prevents any data races if another Task is registering a new atexit hook while the hooks are being evaluated.
2. It defines semantics for what happens if another Task attempts to register another atexit hook _after all the hooks have finished_, and we've proceeded on to the rest of shutdown.
    - Previously, those atexit hooks would be _ignored,_ which violates the user's expectations and violates the "atexit" contract.
    - Now, the attempt to register the atexit hook will **throw an exception,** which ensures that we never break our promise, since the user was never able to register the atexit hook at all.
    - This does mean that users will need to handle the thrown exception and likely do now whatever tear down they were hoping to delay until exit.

* Fix merge conflict
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
domain:multithreading Base.Threads and related functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Lock the atexit_hooks during execution of the hooks on shutdown. JuliaLang/julia
2 participants
@vtjnash @NHDaly