Skip to content

Make in-process implementation threadsafe. #89

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 1 commit into from
Jul 21, 2016
Merged

Conversation

jdm
Copy link
Member

@jdm jdm commented Jul 20, 2016

No description provided.

@jdm
Copy link
Member Author

jdm commented Jul 20, 2016

Tested locally by forcing the in-process implementation to be used in a mac build of Servo.

@nox
Copy link
Contributor

nox commented Jul 20, 2016

@jdm Would it be useful to add an inprocess feature that disables platform-specific implementations if requested?

@jdm
Copy link
Member Author

jdm commented Jul 20, 2016

Maybe, but then I expect we would need to modify a lot of Cargo.toml files to make use of it.

@wafflespeanut
Copy link

@bors-servo r+

@bors-servo
Copy link
Contributor

📌 Commit 30b2024 has been approved by Wafflespeanut

@bors-servo
Copy link
Contributor

⌛ Testing commit 30b2024 with merge 346456b...

bors-servo pushed a commit that referenced this pull request Jul 21, 2016
Make in-process implementation threadsafe.
@bors-servo
Copy link
Contributor

☀️ Test successful - status-appveyor, travis

@bors-servo bors-servo merged commit 30b2024 into master Jul 21, 2016
@wafflespeanut wafflespeanut deleted the threadsafe branch July 21, 2016 19:16
@antrik
Copy link
Contributor

antrik commented Jul 21, 2016

I agree that the inprocess backend simply declaring Sync on OsIpcReceiver is very fishy, and probably needs to be fixed. I don't think wrapping it to actually make it Sync is quite right though: apart from adding measurable overhead that is usually not needed, I believe the receiver becoming Sync is fundamentally wrong -- it just shifts the synchronisation problem elsewhere, and makes it more obscure. Note that mpsc::Receiver itself is Send but !Sync (and the RefCell preserves this), which is an important property of the "single consumer" aspect. Actually using the receiver concurrently from multiple places is bound to introduce race conditions, unless the users make sure all messages are logically independent, or prevent mixups by some kind of synchronisation at a higher level -- in which cases the wrapping should be done individually by these users...

Note that in the Mach (macos) backend, OsIpcReceiver is indeed Send and !Sync AIUI. (It is an integer wrapped in a Cell.)

(The linux variant is technically Sync I think, which is just an accident however, because of the socket FD currently simply being a plain integer -- explicitly declaring it !Sync doesn't appear to break anything. Along with various other problems, this is yet another hint making me lean more and more towards wrapping this one in a Cell as well...)

@jdm
Copy link
Member Author

jdm commented Jul 22, 2016

If it's possible to remove the bits that make it Sync then we should.

@antrik
Copy link
Contributor

antrik commented Aug 1, 2016

@nox a feature to force use of the inprocess backend would be very useful for testing -- I have to work with ugly hacks right now.

Note that this will require some adaptations to the test suite too, as presently some tests make assumptions about the backend in use based on what system we are on.

@nox
Copy link
Contributor

nox commented Aug 2, 2016

@antrik Your wish is my command, my liege. #92

antrik added a commit to antrik/ipc-channel that referenced this pull request Oct 4, 2016
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
servo#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 pushed 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants