Replace rust-websocket with ws-rs in the debugger server. #13258

ejpbruel · 2016-09-13T14:42:32Z

I'm running into several issues with the rust-websocket library. In particular, rust-websocket doesn't seem to make it possible to shut down the websocket server or close individual connections from another thread.

This pull request replaces the rust-websocket library with the ws-rs library. This websocket library is based on mio, and its functionality seems to be closer to what we need. In particular, ws-rs allows shutting down the server from another thread, and handles connection management automatically.

I have updated this pull request to include the changes from my two earlier pull requests, whose changes this pull request subsumes. In particular, one of the earlier pull requests added a way for the constellation to shut down the debugger server, by sending it a ShutdownServer message.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because this is a prototype

This change is

highfive · 2016-09-13T14:42:39Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/Cargo.toml, components/constellation/constellation.rs, components/constellation/lib.rs
@fitzgen: components/debugger/Cargo.toml, components/debugger/lib.rs

bors-servo · 2016-09-23T23:21:33Z

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

ejpbruel · 2016-09-27T11:07:07Z

Review ping!

metajack

This looks great. Main thing to address is bitflags having multiple versions.

metajack · 2016-09-27T12:36:22Z

components/constellation/constellation.rs

@@ -1053,6 +1061,10 @@ impl<Message, LTF, STF> Constellation<Message, LTF, STF>
            warn!("Exit resource thread failed ({})", e);
        }

+        if let Some(ref chan) = self.debugger_chan {
+            shutdown_server(chan);


nit: this would be slightly more clear as debugger::shutdown_server(chan) I think.

metajack · 2016-09-27T12:36:22Z

components/debugger/lib.rs

+
+impl Handler for Connection {
+    fn on_open(&mut self, _: Handshake) -> ws::Result<()> {
+        println!("Connection opened.");


These should probably be debug! or removed.

metajack · 2016-09-27T12:36:22Z

components/debugger/lib.rs

+    }
+
+    fn on_close(&mut self, _: CloseCode, _: &str) {
+        println!("Connection closed.");


debug! or remove

metajack · 2016-09-27T12:36:22Z

components/debugger/lib.rs

-                        panic!("Unexpected message type.");
-                    }
+pub fn start_server(port: u16) -> Sender {
+    println!("Starting server.");


debug! or remove

metajack · 2016-09-27T12:36:22Z

components/debugger/lib.rs

+            Connection { sender: sender }
+        }).unwrap();
+        let sender = socket.broadcaster();
+        spawn_named("debugger-websocket".to_owned(), move || {


How does this thread die?

It dies when the main debugger thread receives a ShutdownServer message, breaks out of its main message loop, and calls sender.shutdown().unwrap(). This will break the websocket server out of its listen loop, allowing the thread to clean itself up.

I meant the thread that is calling listen(). It seems like it will sit forever.

Discussed in IRC and resolved.

metajack · 2016-09-27T12:36:22Z

components/debugger/lib.rs

+}
+
+pub fn shutdown_server(sender: &Sender) {
+    println!("Shutting down server.");


debug! or remove

metajack · 2016-09-27T12:36:22Z

components/servo/Cargo.lock

@@ -183,6 +183,11 @@ source = "registry+https://github.com/rust-lang/crates.io-index"

 [[package]]
 name = "bitflags"
+version = "0.4.0"


Can we avoid multiple versions of bitflags?

I'm not sure how to accomplish this. It looks like the ws library pulls in a different version of the bitflag library than servo itself. How would I go about resolving this?

It needs nix-rust/nix#431. I don't think it is a problem to include 2 versions of bitflags given how small that crate is. It was already an exception in the past anyway.

Ok. I'm fine with extra bitflags then. Can we file a bug upstream to update it at least?

You mean the nix PR that I filed and mentioned in the comment you just replied to? :)

metajack · 2016-09-27T12:36:22Z

ports/cef/Cargo.lock

@@ -156,6 +156,11 @@ source = "registry+https://github.com/rust-lang/crates.io-index"

 [[package]]
 name = "bitflags"
+version = "0.4.0"


Dupe bitflags.

metajack · 2016-09-27T12:36:22Z

servo-tidy.toml

@@ -4,7 +4,7 @@ skip-check-licenses = false

 [ignore]
 # Ignored packages with duplicated versions
-packages = ["lazy_static"]
+packages = ["bitflags", "lazy_static"]


There's no reasoning anywhere for why a dupe bitflags is required. I think that should be explained somewhere or a bug filed and referenced before we allow this.

nox · 2016-09-27T13:48:28Z

nix-rust/nix#431


        Replace rust-websocket with ws-rs in the debugger server.

metajack · 2016-09-30T16:09:45Z

@bors-servo r+

bors-servo · 2016-09-30T16:09:46Z

📌 Commit 9083667 has been approved by metajack

bors-servo · 2016-09-30T16:30:28Z

⌛ Testing commit 9083667 with merge fa760d9...


        Auto merge of #13258 - ejpbruel:ws, r=metajack

Replace rust-websocket with ws-rs in the debugger server.  I'm running into several issues with the rust-websocket library. In particular, rust-websocket doesn't seem to make it possible to shut down the websocket server or close individual connections from another thread. This pull request replaces the rust-websocket library with the ws-rs library. This websocket library is based on mio, and its functionality seems to be closer to what we need. In particular, ws-rs allows shutting down the server from another thread, and handles connection management automatically. I have updated this pull request to include the changes from my two earlier pull requests, whose changes this pull request subsumes. In particular, one of the earlier pull requests added a way for the constellation to shut down the debugger server, by sending it a ShutdownServer message. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because this is a prototype   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13258)

bors-servo · 2016-09-30T16:40:39Z

💔 Test failed - linux-dev

jdm · 2016-09-30T17:35:05Z

error: failed to parse lock file at: /home/servo/buildbot/slave/linux-dev/build/ports/cef/Cargo.lock

bors-servo · 2016-10-18T01:18:33Z

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

nox · 2016-11-07T10:49:54Z

Superseded by #14110.

highfive assigned asajeffrey Sep 13, 2016

highfive added the S-awaiting-review label Sep 13, 2016

jdm added the S-needs-rebase label Sep 13, 2016

ejpbruel force-pushed the ejpbruel:ws branch 3 times, most recently from cf48f2d to 2c2819e Sep 19, 2016

highfive added the S-needs-rebase label Sep 23, 2016

ejpbruel force-pushed the ejpbruel:ws branch from 2c2819e to 1487eb5 Sep 27, 2016

metajack requested changes Sep 27, 2016

View changes

metajack assigned metajack and unassigned asajeffrey Sep 27, 2016

metajack added S-needs-code-changes and removed S-awaiting-review labels Sep 27, 2016

Replace rust-websocket with ws-rs in the debugger server.

Loading status checks…

9083667

ejpbruel force-pushed the ejpbruel:ws branch from 1487eb5 to 9083667 Sep 30, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Sep 30, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Sep 30, 2016

highfive removed the S-awaiting-merge label Sep 30, 2016

highfive added the S-tests-failed label Sep 30, 2016

highfive added the S-needs-rebase label Oct 18, 2016

nox closed this Nov 7, 2016

servo / servo

Replace rust-websocket with ws-rs in the debugger server. #13258

Replace rust-websocket with ws-rs in the debugger server. #13258

ejpbruel commented Sep 13, 2016 •

edited

highfive commented Sep 13, 2016

bors-servo commented Sep 23, 2016

ejpbruel commented Sep 27, 2016

metajack 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.

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.

nox commented Sep 27, 2016

metajack commented Sep 30, 2016

bors-servo commented Sep 30, 2016

bors-servo commented Sep 30, 2016

bors-servo commented Sep 30, 2016

jdm commented Sep 30, 2016

bors-servo commented Oct 18, 2016

nox commented Nov 7, 2016

servo / servo

Join GitHub today

Replace rust-websocket with ws-rs in the debugger server. #13258

Replace rust-websocket with ws-rs in the debugger server. #13258

Conversation

ejpbruel commented Sep 13, 2016 • edited

highfive commented Sep 13, 2016

bors-servo commented Sep 23, 2016

ejpbruel commented Sep 27, 2016

metajack 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.

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.

nox commented Sep 27, 2016

metajack commented Sep 30, 2016

bors-servo commented Sep 30, 2016

bors-servo commented Sep 30, 2016

bors-servo commented Sep 30, 2016

jdm commented Sep 30, 2016

bors-servo commented Oct 18, 2016

nox commented Nov 7, 2016

ejpbruel commented Sep 13, 2016 •

edited