Use automatic thread joining for cargo-watch #3754
Conversation
|
jod_thread (modeled after c++'s jthread) joins in drop. It also propagates panics to the calling thread, which is a more robust behavior. |
| let cmd_send = self.cmd_send.take(); | ||
|
|
||
| // Dropping the sender finishes the thread loop | ||
| drop(cmd_send); |
There was a problem hiding this comment.
I believe the order of drop is crucial here, the cmd_send has to be dropped first such that the loop reading the cargo output terminates abruptly.
I this order guaranteed?
There was a problem hiding this comment.
yes, the drop order of fields is guaranteed in Rust.
|
Ah, looks like this exposed some kind of genuine race! |
| let recv = std::mem::replace(&mut self.message_recv, never()); | ||
|
|
||
| // Dropping the original reciever initiates thread sub-process shutdown | ||
| drop(recv); |
There was a problem hiding this comment.
This is unrelated, but interesting.
This bit of code rubs me the wrong way, because we base cancellation on dropping the receiver, and not sender. This is violates the unidirectional sender->receiver information flow, and is considered "bad".
It took me a while to realize what specific problem we have in this case, but I think there is a problem (mostly theoretical, which doesn't need fixing). Consider a case where cargo is slow at producing messages, and where we are waiting for the next message from Cargo. In this situation, even if we drop the receiver, we won't notice the cancellation until we get next message from Cargo (which might take a while). The "proper" way to do this would be to store a (dummy) reciever in the thread, and do a selec! on the event of receiving cargo message and on the event of dropped sender. That way, we notice cancellation immediately and can cancel Cargo without blocking.
There was a problem hiding this comment.
This reminds me of that dummy receiver being the cancellation token and its appropriate sender the cancellation token source.
There was a problem hiding this comment.
Nice observation, seems a reasonable thing to change at some point.
| let recv = std::mem::replace(&mut self.message_recv, never()); | ||
|
|
||
| // Dropping the original reciever initiates thread sub-process shutdown | ||
| drop(recv); |
There was a problem hiding this comment.
Nice observation, seems a reasonable thing to change at some point.
|
bors try |
tryBuild failed |
|
bors r+ |
Build succeeded |
Make unused states of Reserved unrepresentable In the [previous TB update](rust-lang/miri#3742) we discovered that the existence of `Reserved + !ty_is_freeze + protected` is undesirable. This has the side effect of making `Reserved { conflicted: true, ty_is_freeze: false }` unreachable. As such it is desirable that this state would also be unrepresentable. This PR eliminates the unused configuration by changing ```rs enum PermissionPriv { Reserved { ty_is_freeze: bool, conflicted: bool }, ... } ``` into ```rs enum PermissionPriv { ReservedFrz { conflicted: bool }, ReservedIM, ... } ``` but this is not the only solution and `Reserved(Activable | Conflicted | InteriorMut)` could be discussed. In addition to making the unreachable state not representable anymore, this change has the nice side effect of enabling `foreign_read` to no longer depend explicitly on the `protected` flag. Currently waiting for - `@JoJoDeveloping` to confirm that this is the same representation of `Reserved` as what is being implemented in simuliris, - `@RalfJung` to approve that this does not introduce too much overhead in the trusted codebase.
r? @kiljacken