-
Notifications
You must be signed in to change notification settings - Fork 127
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
Switch to crossbeam-channel #183
Conversation
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.
AIUI, this is still WIP, pending some crossbeam-channel
changes?
A few of my usual nitpicks...
src/platform/inprocess/mod.rs
Outdated
break; | ||
} | ||
if sel.timed_out() { // TODO: this should be any_disconnected | ||
break; |
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 confused: is this actually a viable (though hopefully temporary) workaround somehow -- or is it just plain wrong, but not covered by our test-suite?...
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.
This is a (working) temporary workaround. Let's wait for some changes to crossbeam-channel and then do the right thing here.
src/platform/inprocess/mod.rs
Outdated
@@ -8,7 +8,7 @@ | |||
// except according to those terms. | |||
|
|||
use bincode; | |||
use std::sync::mpsc; | |||
use crossbeam_channel::{self, Receiver, Select, Sender}; |
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.
Personally, I find importing generic names such as Receiver
etc. without a prefix rather confusing... Is that just me?
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.
Well, crossbeam_channel::Receiver
may be a tad too verbose (although mpsc::Receiver
was totally reasonable). Any suggestions? Should we just stick with crossbeam_channel::Receiver
?
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.
Personally, I wouldn't mind the verbose variant... But if you prefer something shorter, you can rename it to a different prefix in the use
statement.
.unwrap() | ||
.get(&self.name) | ||
.unwrap() | ||
.clone(); |
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.
As far as I can tell, this statement got reformatted, although it's entirely unchanged otherwise? I'm not fond of patches being cluttered with this kind of unrelated changes...
(And I'm not even convinced the statement actually becomes more readable in this way -- though that's a matter of taste I guess...)
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.
MpscError
was changed to ChannelError
, but other than that, the code is the same. This formatting was produced by rustfmt. The original is kinda messy - misaligned indentation, missing spaces after commas, etc. I can roll back a few formatting changes...
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 meant just the last line. The function signature reformatting is fine, since you have to touch that anyway -- and the change is a definite improvement there :-)
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.
Actually, in this one instance, I actually do have a minor qualm about the function signature formatting, which I considered mentioning: with the indentation style change, the tuple would now easily fit on one line -- and I tend to think that would be more readable... But then again, I don't really have experience with this style yet -- maybe the more spacey variant is actually more readable after some getting used to?... Not sure. (Which is why I didn't bring it up originally.)
@@ -373,35 +373,37 @@ impl OsIpcSharedMemory { | |||
} | |||
|
|||
#[derive(Debug, PartialEq)] | |||
pub enum MpscError { | |||
pub enum ChannelError { |
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.
Unfortunately, Channel
is rather confusing in this context, as it's not clear that it refers to the underlying crossbeam_channel
mechanism, rather than the IPC channels...
Not sure whether that's a real problem, though.
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.
This seems to be a breaking change since it changes the public API. Should we stick with MpscError
? Or, if we're going to change the name, do you have a better suggestion that ChannelError
?
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.
The change of name and more importantly return type of RouterProxy::route_ipc_receiver_to_*
makes this a breaking change anyway, so no need to worry about that here.
I think that @antrik’s concern is rather that the term "channel" is overloaded. Maybe CrossbeamChannelError
? (Assuming this name does indeed refer to the crate.)
Yes, this is a WIP, pending some |
☔ The latest upstream changes (presumably #188) made this pull request unmergeable. Please resolve the merge conflicts. |
a78a743
to
220e0e5
Compare
@bors-servo try |
Switch to crossbeam-channel Code changes by @stjepang. CC servo/servo#19515
💔 Test failed - status-appveyor |
220e0e5
to
1d7bf81
Compare
@bors-servo try |
Switch to crossbeam-channel Code changes by @stjepang. CC servo/servo#19515
💔 Test failed - status-appveyor |
1d7bf81
to
d0bd3e7
Compare
d0bd3e7
to
cf1acd6
Compare
@bors-servo try |
Switch to crossbeam-channel Code changes by @stjepang. CC servo/servo#19515
☀️ Test successful - status-appveyor, status-travis |
Replace mpsc with crossbeam-channel Follow up on #19515 --- Selecting over multiple channels in `std::sync::mpsc` is not stable and likely never will be: rust-lang/rust#27800 (comment) > It seems the only thing keeping `mpsc_select` around is Servo. crossbeam-channel is designed specifically to replace `std::sync::mpsc` and fix many of its shortcomings: https://github.com/stjepang/rfcs-crossbeam/blob/channel/text/2017-11-09-channel.md This is to be landed together with servo/ipc-channel#183. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21325) <!-- Reviewable:end -->
Replace mpsc with crossbeam-channel Follow up on #19515 --- Selecting over multiple channels in `std::sync::mpsc` is not stable and likely never will be: rust-lang/rust#27800 (comment) > It seems the only thing keeping `mpsc_select` around is Servo. crossbeam-channel is designed specifically to replace `std::sync::mpsc` and fix many of its shortcomings: https://github.com/stjepang/rfcs-crossbeam/blob/channel/text/2017-11-09-channel.md This is to be landed together with servo/ipc-channel#183. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21325) <!-- Reviewable:end -->
Switch to crossbeam-channel Code changes by @stjepang. CC servo/servo#19515
☀️ Test successful - status-appveyor, status-travis |
Replace mpsc with crossbeam-channel Follow up on #19515 --- Selecting over multiple channels in `std::sync::mpsc` is not stable and likely never will be: rust-lang/rust#27800 (comment) > It seems the only thing keeping `mpsc_select` around is Servo. crossbeam-channel is designed specifically to replace `std::sync::mpsc` and fix many of its shortcomings: https://github.com/stjepang/rfcs-crossbeam/blob/channel/text/2017-11-09-channel.md This is to be landed together with servo/ipc-channel#183. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21325) <!-- Reviewable:end -->
Replace mpsc with crossbeam-channel Follow up on #19515 --- Selecting over multiple channels in `std::sync::mpsc` is not stable and likely never will be: rust-lang/rust#27800 (comment) > It seems the only thing keeping `mpsc_select` around is Servo. crossbeam-channel is designed specifically to replace `std::sync::mpsc` and fix many of its shortcomings: https://github.com/stjepang/rfcs-crossbeam/blob/channel/text/2017-11-09-channel.md This is to be landed together with servo/ipc-channel#183. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21325) <!-- Reviewable:end -->
Replace mpsc with crossbeam-channel Follow up on #19515 --- Selecting over multiple channels in `std::sync::mpsc` is not stable and likely never will be: rust-lang/rust#27800 (comment) > It seems the only thing keeping `mpsc_select` around is Servo. crossbeam-channel is designed specifically to replace `std::sync::mpsc` and fix many of its shortcomings: https://github.com/stjepang/rfcs-crossbeam/blob/channel/text/2017-11-09-channel.md This is to be landed together with servo/ipc-channel#183. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21325) <!-- Reviewable:end -->
Replace mpsc with crossbeam-channel Follow up on #19515 --- Selecting over multiple channels in `std::sync::mpsc` is not stable and likely never will be: rust-lang/rust#27800 (comment) > It seems the only thing keeping `mpsc_select` around is Servo. crossbeam-channel is designed specifically to replace `std::sync::mpsc` and fix many of its shortcomings: https://github.com/stjepang/rfcs-crossbeam/blob/channel/text/2017-11-09-channel.md This is to be landed together with servo/ipc-channel#183. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21325) <!-- Reviewable:end -->
Replace mpsc with crossbeam-channel Follow up on #19515 --- Selecting over multiple channels in `std::sync::mpsc` is not stable and likely never will be: rust-lang/rust#27800 (comment) > It seems the only thing keeping `mpsc_select` around is Servo. crossbeam-channel is designed specifically to replace `std::sync::mpsc` and fix many of its shortcomings: https://github.com/stjepang/rfcs-crossbeam/blob/channel/text/2017-11-09-channel.md This is to be landed together with servo/ipc-channel#183. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21325) <!-- Reviewable:end -->
Replace mpsc with crossbeam-channel Follow up on #19515 --- Selecting over multiple channels in `std::sync::mpsc` is not stable and likely never will be: rust-lang/rust#27800 (comment) > It seems the only thing keeping `mpsc_select` around is Servo. crossbeam-channel is designed specifically to replace `std::sync::mpsc` and fix many of its shortcomings: https://github.com/stjepang/rfcs-crossbeam/blob/channel/text/2017-11-09-channel.md This is to be landed together with servo/ipc-channel#183. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21325) <!-- Reviewable:end -->
Replace mpsc with crossbeam-channel Follow up on #19515 --- Selecting over multiple channels in `std::sync::mpsc` is not stable and likely never will be: rust-lang/rust#27800 (comment) > It seems the only thing keeping `mpsc_select` around is Servo. crossbeam-channel is designed specifically to replace `std::sync::mpsc` and fix many of its shortcomings: https://github.com/stjepang/rfcs-crossbeam/blob/channel/text/2017-11-09-channel.md This is to be landed together with servo/ipc-channel#183. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21325) <!-- Reviewable:end -->
Replace mpsc with crossbeam-channel Follow up on #19515 --- Selecting over multiple channels in `std::sync::mpsc` is not stable and likely never will be: rust-lang/rust#27800 (comment) > It seems the only thing keeping `mpsc_select` around is Servo. crossbeam-channel is designed specifically to replace `std::sync::mpsc` and fix many of its shortcomings: https://github.com/stjepang/rfcs-crossbeam/blob/channel/text/2017-11-09-channel.md This is to be landed together with servo/ipc-channel#183. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21325) <!-- Reviewable:end -->
Replace mpsc with crossbeam-channel Follow up on #19515 --- Selecting over multiple channels in `std::sync::mpsc` is not stable and likely never will be: rust-lang/rust#27800 (comment) > It seems the only thing keeping `mpsc_select` around is Servo. crossbeam-channel is designed specifically to replace `std::sync::mpsc` and fix many of its shortcomings: https://github.com/stjepang/rfcs-crossbeam/blob/channel/text/2017-11-09-channel.md This is to be landed together with servo/ipc-channel#183. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21325) <!-- Reviewable:end -->
Replace mpsc with crossbeam-channel Follow up on #19515 --- Selecting over multiple channels in `std::sync::mpsc` is not stable and likely never will be: rust-lang/rust#27800 (comment) > It seems the only thing keeping `mpsc_select` around is Servo. crossbeam-channel is designed specifically to replace `std::sync::mpsc` and fix many of its shortcomings: https://github.com/stjepang/rfcs-crossbeam/blob/channel/text/2017-11-09-channel.md This is to be landed together with servo/ipc-channel#183. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21325) <!-- Reviewable:end -->
Replace mpsc with crossbeam-channel Follow up on #19515 --- Selecting over multiple channels in `std::sync::mpsc` is not stable and likely never will be: rust-lang/rust#27800 (comment) > It seems the only thing keeping `mpsc_select` around is Servo. crossbeam-channel is designed specifically to replace `std::sync::mpsc` and fix many of its shortcomings: https://github.com/stjepang/rfcs-crossbeam/blob/channel/text/2017-11-09-channel.md This is to be landed together with servo/ipc-channel#183. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21325) <!-- Reviewable:end -->
Replace mpsc with crossbeam-channel Follow up on #19515 --- Selecting over multiple channels in `std::sync::mpsc` is not stable and likely never will be: rust-lang/rust#27800 (comment) > It seems the only thing keeping `mpsc_select` around is Servo. crossbeam-channel is designed specifically to replace `std::sync::mpsc` and fix many of its shortcomings: https://github.com/stjepang/rfcs-crossbeam/blob/channel/text/2017-11-09-channel.md This is to be landed together with servo/ipc-channel#183. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21325) <!-- Reviewable:end -->
Replace mpsc with crossbeam-channel Follow up on #19515 --- Selecting over multiple channels in `std::sync::mpsc` is not stable and likely never will be: rust-lang/rust#27800 (comment) > It seems the only thing keeping `mpsc_select` around is Servo. crossbeam-channel is designed specifically to replace `std::sync::mpsc` and fix many of its shortcomings: https://github.com/stjepang/rfcs-crossbeam/blob/channel/text/2017-11-09-channel.md This is to be landed together with servo/ipc-channel#183. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21325) <!-- Reviewable:end -->
Replace mpsc with crossbeam-channel Follow up on #19515 --- Selecting over multiple channels in `std::sync::mpsc` is not stable and likely never will be: rust-lang/rust#27800 (comment) > It seems the only thing keeping `mpsc_select` around is Servo. crossbeam-channel is designed specifically to replace `std::sync::mpsc` and fix many of its shortcomings: https://github.com/stjepang/rfcs-crossbeam/blob/channel/text/2017-11-09-channel.md This is to be landed together with servo/ipc-channel#183. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21325) <!-- Reviewable:end -->
Replace mpsc with crossbeam-channel Follow up on #19515 --- Selecting over multiple channels in `std::sync::mpsc` is not stable and likely never will be: rust-lang/rust#27800 (comment) > It seems the only thing keeping `mpsc_select` around is Servo. crossbeam-channel is designed specifically to replace `std::sync::mpsc` and fix many of its shortcomings: https://github.com/stjepang/rfcs-crossbeam/blob/channel/text/2017-11-09-channel.md This is to be landed together with servo/ipc-channel#183. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21325) <!-- Reviewable:end -->
Replace mpsc with crossbeam-channel Follow up on #19515 --- Selecting over multiple channels in `std::sync::mpsc` is not stable and likely never will be: rust-lang/rust#27800 (comment) > It seems the only thing keeping `mpsc_select` around is Servo. crossbeam-channel is designed specifically to replace `std::sync::mpsc` and fix many of its shortcomings: https://github.com/stjepang/rfcs-crossbeam/blob/channel/text/2017-11-09-channel.md This is to be landed together with servo/ipc-channel#183. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21325) <!-- Reviewable:end -->
Replace mpsc with crossbeam-channel Follow up on #19515 --- Selecting over multiple channels in `std::sync::mpsc` is not stable and likely never will be: rust-lang/rust#27800 (comment) > It seems the only thing keeping `mpsc_select` around is Servo. crossbeam-channel is designed specifically to replace `std::sync::mpsc` and fix many of its shortcomings: https://github.com/stjepang/rfcs-crossbeam/blob/channel/text/2017-11-09-channel.md This is to be landed together with servo/ipc-channel#183. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21325) <!-- Reviewable:end -->
Replace mpsc with crossbeam-channel Follow up on #19515 --- Selecting over multiple channels in `std::sync::mpsc` is not stable and likely never will be: rust-lang/rust#27800 (comment) > It seems the only thing keeping `mpsc_select` around is Servo. crossbeam-channel is designed specifically to replace `std::sync::mpsc` and fix many of its shortcomings: https://github.com/stjepang/rfcs-crossbeam/blob/channel/text/2017-11-09-channel.md This is to be landed together with servo/ipc-channel#183. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/21325) <!-- Reviewable:end -->
Code changes by @stjepang.
CC servo/servo#19515