Update crossbeam-channel to 0.3 #22142

Eijebong · 2018-11-07T21:20:53Z

servo/ipc-channel#210

This change is

highfive · 2018-11-07T21:20:58Z

Heads up! This PR modifies the following files:

@jgraham: components/webdriver_server/Cargo.toml, components/webdriver_server/lib.rs
@emilio: components/style/lib.rs, components/layout/animation.rs, components/style/context.rs, components/style/Cargo.toml, components/layout/Cargo.toml and 2 more
@KiChjang: components/script/Cargo.toml, components/net/lib.rs, components/script/dom/testworkletglobalscope.rs, components/script_traits/lib.rs, components/net/tests/fetch.rs and 27 more
@asajeffrey: components/script/Cargo.toml, components/script/dom/testworkletglobalscope.rs, components/constellation/network_listener.rs, components/script/lib.rs, components/script/dom/abstractworkerglobalscope.rs and 24 more
@cbrewster: components/constellation/network_listener.rs, components/constellation/lib.rs, components/constellation/pipeline.rs, components/constellation/Cargo.toml, components/constellation/constellation.rs and 1 more
@edunham: servo-tidy.toml
@paulrouget: components/servo/Cargo.toml, components/compositing/lib.rs, components/constellation/network_listener.rs, components/compositing/compositor_thread.rs, ports/servo/Cargo.toml and 8 more

Eijebong · 2018-11-07T21:21:04Z

cc @stjepang

Eijebong · 2018-11-07T21:22:09Z

components/script/dom/abstractworkerglobalscope.rs

-        recv(timer_event_port.select(), msg) => worker_scope.from_timer_msg(msg.unwrap()),
-        recv(devtools_port, msg) => worker_scope.from_devtools_msg(msg.unwrap()),
+        recv(timer_event_port) -> msg => worker_scope.from_timer_msg(msg.unwrap()),
+        // TODO: recv(devtools_port) -> msg => worker_scope.from_devtools_msg(msg.unwrap()),


So here is the only thing I didn't know how to do. select! doesn't support recv(Option<Receiver>) anymore. Do I have to write that with Select then ?

You could do that with Select, but you don't have to. There's another way:

recv(devtools_port.unwrap_or(&crossbeam_channel::never())) -> msg => ...

See here for documentation on never.

I removed the ability to use Option<&Receiver<_>> in select! because it was an ugly hack that abuses traits and doesn't work as well as I hoped for. Maybe I'll add it again sometime in the future when we get specialization in Rust.

stjepang

Looks like a good start! I left some comments inline.

stjepang · 2018-11-08T10:42:27Z

components/net/tests/fetch.rs

@@ -9,6 +9,7 @@ use crate::{
    create_embedder_proxy, fetch, make_server, make_ssl_server, new_fetch_context,
    DEFAULT_USER_AGENT,
 };
+use crossbeam_channel::{channel, Sender};


This function is called unbounded, not channel. The same error appears in some other files, which is why CI is currently failing.

Oh woops, didn't think about unit tests

stjepang · 2018-11-08T10:42:27Z

components/script/dom/abstractworkerglobalscope.rs

-        recv(timer_event_port.select(), msg) => worker_scope.from_timer_msg(msg.unwrap()),
-        recv(devtools_port, msg) => worker_scope.from_devtools_msg(msg.unwrap()),
+        recv(timer_event_port) -> msg => worker_scope.from_timer_msg(msg.unwrap()),
+        // TODO: recv(devtools_port) -> msg => worker_scope.from_devtools_msg(msg.unwrap()),


You could do that with Select, but you don't have to. There's another way:

recv(devtools_port.unwrap_or(&crossbeam_channel::never())) -> msg => ...

See here for documentation on never.

I removed the ability to use Option<&Receiver<_>> in select! because it was an ugly hack that abuses traits and doesn't work as well as I hoped for. Maybe I'll add it again sometime in the future when we get specialization in Rust.

bors-servo · 2018-11-08T11:57:54Z

☔ The latest upstream changes (presumably #22133) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo · 2018-11-09T21:48:41Z

☔ The latest upstream changes (presumably #22098) made this pull request unmergeable. Please resolve the merge conflicts.

Eijebong · 2018-11-09T23:56:21Z

@bors-servo try

bors-servo · 2018-11-09T23:56:26Z

⌛ Trying commit 9ac425b with merge fd0cde1...


        Auto merge of #22142 - Eijebong:crossbeamup, r=<try>

Update crossbeam-channel to 0.3 servo/ipc-channel#210  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22142)

bors-servo · 2018-11-10T00:00:20Z

💔 Test failed - status-taskcluster

Eijebong · 2018-11-10T00:34:38Z

@stjepang Should be ready to go now

stjepang

A few more nits. We can merge after they get resolved.

stjepang · 2018-11-10T10:48:22Z

components/constellation/timer_scheduler.rs

@@ -70,9 +70,9 @@ impl TimerScheduler {
                    }
                    // Look to see if there are any incoming events
                    select! {
-                        recv(req_receiver, msg) => match msg {
+                        recv(req_receiver) -> msg => match msg {


Let's rewrite this whole select! block using match req_receiver.try_recv():

// Look to see if there are any incoming events match req_receiver.try_recv() { // If there is an event, add it to the priority queue Ok(TimerSchedulerMsg::Request(req)) => { let TimerEventRequest(_, _, _, delay) = req; let schedule = Instant::now() + Duration::from_millis(delay.get()); let event = ScheduledEvent { request: req, for_time: schedule, }; scheduled_events.push(event); }, // If there is no incoming event, park the thread, // it will either be unparked when a new event arrives, // or by a timeout. Err(TryRecvError::Empty) => match scheduled_events.peek() { None => thread::park(), Some(event) => thread::park_timeout(event.for_time - now), }, // If the channel is closed or we are shutting down, we are done. Ok(TimerSchedulerMsg::Exit) | Err(TryRecvError::Disconnected) => break, }

stjepang · 2018-11-10T10:48:22Z

components/script/dom/abstractworkerglobalscope.rs

@@ -68,7 +68,7 @@ impl ScriptChan for WorkerThreadWorkerChan {

 impl ScriptPort for Receiver<DedicatedWorkerScriptMsg> {
    fn recv(&self) -> Result<CommonScriptMsg, ()> {
-        let common_msg = match self.recv() {
+        let common_msg = match self.recv().ok() {


Nit: Remove this .ok() and change below:

Some to Ok

None to Err(_)

stjepang · 2018-11-10T10:48:22Z

components/script/dom/abstractworkerglobalscope.rs

-            None => match timer_event_port.try_recv() {
-                None => match devtools_port.and_then(|port| port.try_recv()) {
+            None => match timer_event_port.try_recv().ok() {
+                None => match devtools_port.and_then(|port| port.try_recv().ok()) {


Nit: Remove those two .ok()s and change:

Some to Ok

None to Err(_)

stjepang · 2018-11-10T10:48:22Z

components/script/dom/paintworkletglobalscope.rs

@@ -446,10 +445,10 @@ impl PaintWorkletGlobalScope {
                    .unwrap_or(10u64);

                select! {
-                    recv(base_channel::after(Duration::from_millis(timeout))) => {
+                    recv(crossbeam_channel::after(Duration::from_millis(timeout))) -> _ => {


Let's replace this select! with:

receiver.recv_timeout(timeout_duration) .map_err(|e| PaintWorkletError::from(e))

And then restore this impl, which was removed in the PR that migrated from mpsc to crossbeam-channel v0.2:
https://github.com/gterzian/servo/blob/e784f5a9f7a4c3eec92b946b0debd4cbcb942ae7/components/script_traits/lib.rs#L807-L811

stjepang · 2018-11-10T10:48:22Z

components/script/dom/window.rs

@@ -1498,7 +1498,7 @@ impl Window {
        debug!("script: layout forked");

        let complete = select! {
-            recv(join_port.select(), msg) => if let Some(reflow_complete) = msg {
+            recv(join_port) -> msg => if let Ok(reflow_complete) = msg {


We can replace this select! with:

let complete = match join_port.try_recv() { Err(Empty) => { info!("script: waiting on layout"); join_port.recv().unwrap() } Ok(reflow_complete) => reflow_complete, Err(Disconnected) => { panic!("Layout thread failed while script was waiting for a result."); } };

stjepang · 2018-11-10T10:48:22Z

components/script/serviceworker_manager.rs

-            recv(self.own_port.select(), msg) => msg.map(Message::FromConstellation),
-            recv(self.resource_receiver.select(), msg) => msg.map(Message::FromResource),
+            recv(self.own_port) -> msg => msg.ok().map(Message::FromConstellation),
+            recv(self.resource_receiver) -> msg => msg.ok().map(Message::FromResource),


Let's revert this whole method to (roughly) what it used to be:

fn receive_message(&mut self) -> Result<Message, RecvError> { select! { recv(self.own_port) -> msg => msg.map(Message::FromConstellation), recv(self.resource_receiver) -> msg => msg.map(Message::FromResource), } }

There's a Some in handle_message that'll need to be changed to Ok.

stjepang · 2018-11-10T10:48:22Z

components/script/task_queue.rs

@@ -110,12 +110,12 @@ impl<T: QueuedTaskConversion> TaskQueue<T> {

    /// Reset the queue for a new iteration of the event-loop,
    /// returning the port about whose readiness we want to be notified.
-    pub fn select(&self) -> &base_channel::Receiver<T> {
+    pub fn select(&self) -> &crossbeam_channel::Receiver<T> {


Methods recv and try_recv below this select method used to return Result<T, ()>. Perhaps we should restore that.

stjepang · 2018-11-10T10:48:22Z

components/script/script_thread.rs

    }
 }

 impl ScriptPort for Receiver<MainThreadScriptMsg> {
    fn recv(&self) -> Result<CommonScriptMsg, ()> {
-        match self.recv() {
+        match self.recv().ok() {


Nit: remove this .ok() and replace Some/None with Ok/Err.

stjepang · 2018-11-10T10:48:22Z

components/script/script_thread.rs

    }
 }

 impl ScriptPort for Receiver<(TrustedWorkerAddress, MainThreadScriptMsg)> {
    fn recv(&self) -> Result<CommonScriptMsg, ()> {
-        match self.recv().map(|(_, msg)| msg) {
+        match self.recv().map(|(_, msg)| msg).ok() {


Nit: remove this .ok() and replace Some/None with Ok/Err.

stjepang · 2018-11-10T10:48:22Z

components/script/script_thread.rs

-                            None => match self.image_cache_port.try_recv() {
+                    None => match self.timer_event_port.try_recv().ok() {
+                        None => match self.devtools_port.try_recv().ok() {
+                            None => match self.image_cache_port.try_recv().ok() {


Nit: remove these .ok()s and replace Some/None with Ok/Err.

bors-servo · 2018-11-10T18:24:35Z

☔ The latest upstream changes (presumably #22083) made this pull request unmergeable. Please resolve the merge conflicts.

Eijebong · 2018-11-17T17:40:02Z

Done.

@bors-servo try=wpt

bors-servo · 2018-11-18T18:31:43Z

🔒 Merge conflict

bors-servo · 2018-11-18T18:33:12Z

☔ The latest upstream changes (presumably #22209) made this pull request unmergeable. Please resolve the merge conflicts.


        Update crossbeam-channel to 0.3

Eijebong · 2018-11-18T18:33:30Z

Rebased

jdm · 2018-11-18T18:38:46Z

@bors-servo r+

bors-servo · 2018-11-18T18:38:46Z

📌 Commit 9a7eeb3 has been approved by jdm

bors-servo · 2018-11-18T19:39:59Z

⌛ Testing commit 9a7eeb3 with merge eb3af18...


        Auto merge of #22142 - Eijebong:crossbeamup, r=jdm

Update crossbeam-channel to 0.3 servo/ipc-channel#210  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22142)

bors-servo · 2018-11-18T20:38:11Z

☀️ Test successful - android, android-mac, android-x86, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, magicleap, status-taskcluster, windows-msvc-dev
Approved by: jdm
Pushing eb3af18 to master...


        Bug 1511811 - Update crossbeam-channel to 0.3.

Cherry-picks servo/servo#22142


        Bug 1511811 - Update crossbeam-channel to 0.3.

Cherry-picks servo/servo#22142


        Bug 1511811 - Update crossbeam-channel to 0.3.

Cherry-picks servo/servo#22142


        Bug 1511811 - Update crossbeam-channel to 0.3.

Cherry-picks servo/servo#22142

highfive assigned emilio Nov 7, 2018

highfive added the S-awaiting-review label Nov 7, 2018

Eijebong reviewed Nov 7, 2018

View changes

Eijebong force-pushed the Eijebong:crossbeamup branch from 1f57056 to 7dbc1db Nov 7, 2018

stjepang suggested changes Nov 8, 2018

View changes

highfive added the S-needs-rebase label Nov 8, 2018

Eijebong force-pushed the Eijebong:crossbeamup branch 2 times, most recently from db426a4 to 65573a1 Nov 8, 2018

Eijebong force-pushed the Eijebong:crossbeamup branch from 65573a1 to 9ac425b Nov 9, 2018

Eijebong changed the title ~~WIP: Update crossbeam-channel to 0.3~~ Update crossbeam-channel to 0.3 Nov 9, 2018

highfive added the S-tests-failed label Nov 10, 2018

Eijebong force-pushed the Eijebong:crossbeamup branch from 9ac425b to f51e4bf Nov 10, 2018

highfive removed the S-tests-failed label Nov 10, 2018

stjepang suggested changes Nov 10, 2018

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Nov 15, 2018

Eijebong force-pushed the Eijebong:crossbeamup branch from f51e4bf to 031287d Nov 17, 2018

highfive added S-awaiting-review and removed S-needs-code-changes labels Nov 17, 2018

highfive added the S-awaiting-merge label Nov 18, 2018

highfive added S-needs-rebase and removed S-awaiting-merge labels Nov 18, 2018

Update crossbeam-channel to 0.3

Loading status checks…

9a7eeb3

Eijebong force-pushed the Eijebong:crossbeamup branch from f9007ed to 9a7eeb3 Nov 18, 2018

highfive added the S-awaiting-review label Nov 18, 2018

highfive assigned jdm and unassigned emilio Nov 18, 2018

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Nov 18, 2018

bors-servo merged commit 9a7eeb3 into servo:master Nov 18, 2018
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Nov 18, 2018

This was referenced Nov 18, 2018

Add background hang monitor #21673

Merged

Update MPL license to https #22198

Merged

Remove sync constellation -> embedder communication #22121

Merged

stjepang mentioned this pull request Nov 18, 2018

Update crossbeam-channel to v0.3 #22108

Closed

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 3, 2018

Bug 1511811 - Update crossbeam-channel to 0.3.

768dd10

Cherry-picks servo/servo#22142

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 3, 2018

Bug 1511811 - Update crossbeam-channel to 0.3.

87addfd

Cherry-picks servo/servo#22142

mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Dec 4, 2018

Bug 1511811 - Update crossbeam-channel to 0.3.

34b97bf

Cherry-picks servo/servo#22142

mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Dec 4, 2018

Bug 1511811 - Update crossbeam-channel to 0.3.

dbdacdd

Cherry-picks servo/servo#22142

stjepang mentioned this pull request Jan 30, 2019

Panic on `send` in channels by default? crossbeam-rs/crossbeam#314

Closed

servo / servo

Update crossbeam-channel to 0.3 #22142

Update crossbeam-channel to 0.3 #22142

Eijebong commented Nov 7, 2018 •

edited by SimonSapin

highfive commented Nov 7, 2018

Eijebong commented Nov 7, 2018

This comment has been minimized.

This comment has been minimized.

stjepang left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Nov 8, 2018

bors-servo commented Nov 9, 2018

Eijebong commented Nov 9, 2018

bors-servo commented Nov 9, 2018

bors-servo commented Nov 10, 2018

Eijebong commented Nov 10, 2018

stjepang left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Nov 10, 2018

Eijebong commented Nov 17, 2018

bors-servo commented Nov 18, 2018

bors-servo commented Nov 18, 2018

Eijebong commented Nov 18, 2018

jdm commented Nov 18, 2018

bors-servo commented Nov 18, 2018

bors-servo commented Nov 18, 2018

bors-servo commented Nov 18, 2018

servo / servo

Join GitHub today

Update crossbeam-channel to 0.3 #22142

Update crossbeam-channel to 0.3 #22142

Conversation

Eijebong commented Nov 7, 2018 • edited by SimonSapin

highfive commented Nov 7, 2018

Eijebong commented Nov 7, 2018

This comment has been minimized.

This comment has been minimized.

stjepang left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Nov 8, 2018

bors-servo commented Nov 9, 2018

Eijebong commented Nov 9, 2018

bors-servo commented Nov 9, 2018

bors-servo commented Nov 10, 2018

Eijebong commented Nov 10, 2018

stjepang left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented Nov 10, 2018

Eijebong commented Nov 17, 2018

bors-servo commented Nov 18, 2018

bors-servo commented Nov 18, 2018

Eijebong commented Nov 18, 2018

jdm commented Nov 18, 2018

bors-servo commented Nov 18, 2018

bors-servo commented Nov 18, 2018

bors-servo commented Nov 18, 2018

Eijebong commented Nov 7, 2018 •

edited by SimonSapin