-
Notifications
You must be signed in to change notification settings - Fork 2
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
Fix the concurrency semantics for Gc<T>
.
#54
Conversation
This was included originally because of concerns about finalizing a non-Sync `T` value off-thread. If a value is `Sync`, it means that it is safe for two threads to access its data simultaneuosly: in other words, it has some synchronization guarantees. A common example is `RefCell`, which is safe to send between threads, but is marked `!Sync` because the dynamic borrow checking is not atomic. Finalizing `!Sync` values no longer problematic, because even though finalizers are run off-thread, they will run when the object is dead [1], so there is no chance of a data race. [1]: softdevteam/alloy#30
This improves the ergonomics of `Gc<T>`, allowing nested `Gc` types without the need for introducing unsafe newtype workarounds [1].
let gc = Gc::new(String::from("Hello world!")); | ||
|
||
// Wait a bit before dying, ensuring that the thread stays alive long enough | ||
// cross the force_gc call. |
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.
s/cross/across/ (assuming there's nothing to do with anger here :p ). I also wonder if we should make the test more robust by waiting on a mutex / bool or something? The problem is that there's no guarantee that force_gc
will happen in 10ms.
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.
ha, I was actually missing a "to" after enough there. Yeah it's a bit flakey, 17c25c1 should make it less so. It's hard not to have false-negatives with these tests, but if the test does fail, something is certainly wrong.
gc_tests/tests/multithreaded.rs
Outdated
@@ -20,11 +20,17 @@ impl Drop for PanicOnDrop { | |||
|
|||
} | |||
|
|||
// This can race, but it's benign because it's only read from the main thread, | |||
// and we only care that _at some point_ it gets flipped. | |||
static mut NO_CHILD_EXISTS: bool = true; |
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.
Let's make this an AtomicBool
with Ordering::Relaxed
and then I think we're actually OK.
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.
Ok, I think it's probably safer to use Sequential Consistency because I don't want the load in the while loop to be reordered below the call to force_gc
.
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.
Good point!
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.
Try feba5ed
Please squash. |
feba5ed
to
17cf277
Compare
Squashed |
bors r+ |
Build succeeded: |
This makes two major changes to the API:
T: Sync
forGc<T>
.Gc<T> : Send + Sync
ifT: Send + Sync
, fixing the ergonomic problems raised in Gc type does not implement Send or Sync #49.Sync
's purpose is to ensure that two threads can access the data inT
in a thread-safe way. In other words, it implies thatT
has synchronisation guarantees. Originally, this was added as a constraint onT
because any finalizer forT
would run on a separate thread. However, it's now safe to remove this as a constraint because softdevteam/alloy#30 guarantees that a finalizer won't run early. This means that even without synchronized access, a race can't happen, because it's impossible for the finalizer to accessT
's data while it's still in use by the mutator.However, even though
Gc<T>
can now implementSend
-- thanks to multi-threaded collection support --Gc<T>
still requires thatT: Send
, becauseT
could be a type with shared ownership which aliases. This is necessary because off-thread finalization could mutate shared memory without synchronisation. An example usingRc
makes this clear:Since finalizing any non-
Send
value can cause UB, we still disallow the construction ofGc<T: !Send>
completely. This is certainly the most conservative approach. There are others:Send
values. This is valid, since finalizers are not guaranteed to run. However, it's not exactly practical, it would mean that anyGc<Rc<...>>
type structure would always leak.!Send
values on their mutator thread. This is really dangerous in the general case, because if any locks are held on the shared data by the mutator, this will deadlock (it's basically a variant of the async-signal-safe problem). However, ifT
is!Send
, deadlocking is unlikely [although not impossible!], and could be an acceptable compromise. It's out of the scope of this PR, but it's something I've been playing around with.