Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upinprocess: Don't wrap `OsIpcReceiver` to make it `Sync` #107
Conversation
Wrap the inprocess receiver in a simple `RefCell<>` instead of `Arc<Mutex<>>` (again). Adding the mutex here is not a good idea: it only adds overhead and potential errors, since not all of the other backends have a `Sync` receiver either; nor does the `mpsc` mechanism `ipc-channel` is modelled after. In fact it fundamentally violates the "single receiver" aspect of `mpsc`, as outlined in #89 (comment) Users can still wrap it explicitly if they need to, and know it doesn't introduce incorrect behaviour in the specific use case. This change mostly reverts the `Receiver` part of 30b2024 ; it doesn't re-introduce the explicit `Sync` declaration for `Receiver` though, as that was/is clearly not true without the Mutex -- nor really desirable, as explained above. This change also doesn't touch the `Sender` part, which is a different story.
|
@bors-servo: r+ |
|
|
bors-servo
added a commit
that referenced
this pull request
Oct 4, 2016
inprocess: Don't wrap `OsIpcReceiver` to make it `Sync` Wrap the inprocess receiver in a simple `RefCell<>` instead of `Arc<Mutex<>>` (again). Adding the mutex here is not a good idea: it only adds overhead and potential errors, since not all of the other backends have a `Sync` receiver either; nor does the `mpsc` mechanism `ipc-channel` is modelled after. In fact it fundamentally violates the "single receiver" aspect of `mpsc`, as outlined in #89 (comment) Users can still wrap it explicitly if they need to, and know it doesn't introduce incorrect behaviour in the specific use case. This change mostly reverts the `Receiver` part of 30b2024 ; it doesn't re-introduce the explicit `Sync` declaration for `Receiver` though, as that was/is clearly not true without the Mutex -- nor really desirable, as explained above. This change also doesn't touch the `Sender` part, which is a different story.
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
antrik commentedOct 4, 2016
Wrap the inprocess receiver in a simple
RefCell<>instead ofArc<Mutex<>>(again).Adding the mutex here is not a good idea: it only adds overhead and
potential errors, since not all of the other backends have a
Syncreceiver either; nor does the
mpscmechanismipc-channelis modelledafter. In fact it fundamentally violates the "single receiver" aspect of
mpsc, as outlined in#89 (comment)
Users can still wrap it explicitly if they need to, and know it doesn't
introduce incorrect behaviour in the specific use case.
This change mostly reverts the
Receiverpart of30b2024 ; it doesn't re-introduce the
explicit
Syncdeclaration forReceiverthough, as that was/isclearly not true without the Mutex -- nor really desirable, as explained
above. This change also doesn't touch the
Senderpart, which is adifferent story.