-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Lock the atexit_hooks during execution of the hooks on shutdown. #49868
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
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
34a8e44
Lock the atexit_hooks during execution of the hooks on shutdown.
NHDaly e31b144
Disable atexit() when shutting down.
NHDaly b9f7bf5
Make lock more precise; allow atexit() inside atexit hook.
NHDaly 166d20d
Add paragraph describing the new error behavior to docstring.
NHDaly d033f62
Remove redundant test
NHDaly 60f27f6
Simplify atexit locking
NHDaly edf3da2
Improve global bool declaration
NHDaly 95892be
Nicer global boolean
NHDaly File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vtjnash: Oh, wait, this needs to be
volatile, right? Since it's used inside a while loop across multiple threads?So this should be an
atomic, since we don't have the ability to do volatile without atomic, right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking this might explain the failure you linked:
https://buildkite.com/julialang/julia-master/builds/24379#01886d90-17c9-48d9-9710-b2b18d515682
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
volatileis only meaningful for special memory mapped hardware registers, and x86 doesn't have any of thoseThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that true? Isn't it also needed to prevent global variables from getting pulled into either a register or a stack-local variable and thus breaking coordination across threads?
I think we've seen this in julia code before, that unless you mark the variable as an
Atomicit ends up inlined as a local into the function body.Here's an example:
And you can see that this breaks coordination across tasks:
Whereas by marking this variable as an
Atomic, it fixes the inlining of the variable, and its value is fetched on every iteration of the loop:And now this issue is fixed as well:
^^ So I do think there is an issue in the code as written. I am pretty sure that this issue is part of what is encompassed by the term "volatility", but using whatever name we want to use for it, it's an issue, yeah?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See also: https://www.geeksforgeeks.org/understanding-volatile-qualifier-in-c/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although in this situation, it's also fixed by using a lock, which I guess introduces the needed barriers... 🤔
So are you saying that the lock like that should be sufficient in all cases, and we don't also need to mark the variable atomic, to get it to become volatile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think what Jameson meant is that the notion of "volatile" is irrelevant here. The notion that is of import is "atomicity" (https://en.cppreference.com/w/c/language/atomic).
Either by marking the Bool
Atomicor by protecting it with a lock. Before C11 volatile and atomic were often confounded.From https://en.cppreference.com/w/c/language/volatile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aha. Thank you both! That's clear now, then. 👍 Thanks for educating me!
So, a julia ReentrantLock is enough to also provide the LLVM / C concept of atomicity? We can guarantee that the updates to the value will be reflected across threads?
Now that we're discussing it, the answer seems to obviously be yes, otherwise how would anything work... OKAY thanks this is helpful.