stop client websocket close echoing server close #10382

bobthekingofegypt · 2016-04-03T13:24:20Z

Client initiated close requests should send a close message to the
server that the server will echo back to complete the process. Servo
should not then echo the servers close request back again to the server,
this guard stops servo from echoing a server close request if the
process was initiated by the client.

Tracked in #9803 (comment)

This change is

highfive · 2016-04-03T13:24:25Z

Heads up! This PR modifies the following files:

@KiChjang: components/net/websocket_loader.rs

highfive · 2016-04-03T13:24:26Z

Warning

These commits modify net code, but no tests are modified. Please consider adding a test!

KiChjang · 2016-04-03T16:45:58Z

components/net/websocket_loader.rs

@@ -123,7 +126,9 @@ pub fn init(connect: WebSocketCommunicate, connect_data: WebSocketConnectData, c
                    },
                    Type::Pong => continue,
                    Type::Close => {
-                        ws_sender_incoming.lock().unwrap().send_message(&message).unwrap();
+                        if client_initiated_close_incoming.load(Ordering::SeqCst) == false { 


nit: if !client_initiated_close_incoming.load(Ordering::SeqCst) {

metajack · 2016-04-04T16:31:11Z

r=me with @KiChjang's comment addressed

Review status: 0 of 1 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Comments from Reviewable

metajack · 2016-04-04T16:31:37Z

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Comments from Reviewable

bobthekingofegypt · 2016-04-04T18:40:48Z

@KiChjang's comment addressed.

metajack · 2016-04-04T18:47:51Z

@bors-servo r+

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

bors-servo · 2016-04-04T18:47:52Z

📌 Commit cbfc36d has been approved by metajack

metajack · 2016-04-04T18:48:37Z

@bors-servo r-

Please squash and we'll get this merged.

+S-needs-squash

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

bobthekingofegypt · 2016-04-04T20:28:41Z

Sorry, I'll need to add the guard around https://github.com/bobthekingofegypt/servo/blob/fix-websocket-close/components/net/websocket_loader.rs#L160 as well. As it's still possible to send two close messages. Will get it done tomorrow.

bobthekingofegypt · 2016-04-10T12:36:00Z

Sorry for the delay, I added the check around both incoming and outgoing. If it's ok, I'll squash all the commits.

metajack · 2016-04-13T19:23:28Z

Looks good, aside from the comment I left. You can go ahead and squash it at the same time you address that.

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

components/net/websocket_loader.rs, line 129 [r3] (raw file):
If we weren't waiting for the close to come in, we now set that we are. When we aren't waiting for a close, getting a close needs a ack. By sending this ack, we should not be getting an ack to our ack, nor did we send a client intiated close. So I'm not sure why you are toggling the bool in this case.

Comments from Reviewable

perlun · 2016-04-22T19:59:23Z

@bobthekingofegypt - any progress on this one?

bobthekingofegypt · 2016-04-23T14:19:38Z

I can't really use computers at the moment due to an issue with my eye. So I can't really deal with this right now. I hope to be back to normal in a few weeks.

In response to your comment @metajack the reason that the boolean is checked and updated on both incoming and outgoing threads is due to the current threading.
From memory without checking anything it is to do with the websocket server initiating a close and the user script on servo side calling close at the same time, happens a lot on the test cases.
Whatever happens you only want to send one close whether it is an initiator or an echo, the boolean check and update on both means only one close with ever be sent regardless of the order the threads are executed.

I can write up a better explanation when I'm back but I'm just rushing this comment for now. If someone else wants to wrap this up feel free.

bobthekingofegypt · 2016-05-06T17:59:16Z

components/net/websocket_loader.rs, line 129 [r3] (raw file):
Sorry, I think I misunderstood this when I replied in the PR. I'm still not sure I fully understand. I think I may have confused things by naming the variable initiated_close rather than something like sent_close.
I flip the guard here as well as down below because I only want to send a close once, regardless of whether it is an echo or an instigator. If I don't flip the guard here then a JS call to close the websocket can still trigger another close event through line 155 as the state on dom/websocket side is not yet marked as closed or closing.

Comments from Reviewable

jdm · 2016-06-08T17:08:42Z

The code and explanation makes sense to me. @bobthekingofegypt if you squash the commits, I'll merge it :)

metajack · 2016-06-08T20:50:35Z

I'm not sure how I forgot to comment here. I could have sworn I wrote out an r=me.

In any case, I apologize for the delay and I agree with jdm.


        check close on both incoming/outgoing websocket

Need to make sure close is only sent to the server once, either from a client initiation or from a server echo. This adds the sent check to both incoming and outgoing threads.

highfive · 2016-06-10T12:48:54Z

New code was committed to pull request.

jdm · 2016-06-10T13:06:21Z

@bors-servo: r=metajack

bors-servo · 2016-06-10T13:06:22Z

📌 Commit 27b4ac6 has been approved by metajack

bors-servo · 2016-06-10T13:49:54Z

⌛ Testing commit 27b4ac6 with merge 96f3404...


        Auto merge of #10382 - bobthekingofegypt:fix-websocket-close, r=metajack

stop client websocket close echoing server close Client initiated close requests should send a close message to the server that the server will echo back to complete the process. Servo should not then echo the servers close request back again to the server, this guard stops servo from echoing a server close request if the process was initiated by the client. Tracked in #9803 (comment)  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10382)

bors-servo · 2016-06-10T15:06:27Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

highfive assigned metajack Apr 3, 2016

highfive added the S-awaiting-review label Apr 3, 2016

KiChjang reviewed Apr 3, 2016
View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Apr 4, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Apr 4, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Apr 4, 2016

metajack added the S-needs-squash label Apr 4, 2016

jdm removed the S-awaiting-merge label Apr 4, 2016

bobthekingofegypt mentioned this pull request Apr 4, 2016

Intermittent pass in /websockets/cookies/005.html #9772

Open

highfive added the S-awaiting-review label Apr 10, 2016

jdm removed the S-needs-squash label Apr 13, 2016

jdm unassigned metajack Jun 8, 2016

jdm self-assigned this Jun 8, 2016

larsbergstrom assigned metajack Jun 8, 2016

jdm added S-needs-squash and removed S-awaiting-review labels Jun 8, 2016

jdm unassigned metajack Jun 8, 2016

check close on both incoming/outgoing websocket

Loading status checks…

27b4ac6

Need to make sure close is only sent to the server once, either from a client initiation or from a server echo. This adds the sent check to both incoming and outgoing threads.

bobthekingofegypt force-pushed the bobthekingofegypt:fix-websocket-close branch from 97c58fc to 27b4ac6 Jun 10, 2016

highfive added the S-awaiting-review label Jun 10, 2016

highfive assigned metajack and unassigned jdm Jun 10, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-squash labels Jun 10, 2016

bors-servo merged commit 27b4ac6 into servo:master Jun 10, 2016
2 checks passed

2 checks passed

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Jun 10, 2016

jdm mentioned this pull request Jun 11, 2016

Intermittent timeout of lots of websocket tests #11725

Closed

servo / servo

stop client websocket close echoing server close #10382

stop client websocket close echoing server close #10382

bobthekingofegypt commented Apr 3, 2016

highfive commented Apr 3, 2016

highfive commented Apr 3, 2016

This comment has been minimized.

metajack commented Apr 4, 2016

metajack commented Apr 4, 2016

bobthekingofegypt commented Apr 4, 2016

metajack commented Apr 4, 2016

bors-servo commented Apr 4, 2016

metajack commented Apr 4, 2016

bobthekingofegypt commented Apr 4, 2016

bobthekingofegypt commented Apr 10, 2016

metajack commented Apr 13, 2016

perlun commented Apr 22, 2016

bobthekingofegypt commented Apr 23, 2016

bobthekingofegypt commented May 6, 2016

jdm commented Jun 8, 2016

metajack commented Jun 8, 2016

highfive commented Jun 10, 2016

jdm commented Jun 10, 2016

bors-servo commented Jun 10, 2016

bors-servo commented Jun 10, 2016

bors-servo commented Jun 10, 2016

servo / servo

Join GitHub today

stop client websocket close echoing server close #10382

stop client websocket close echoing server close #10382

Conversation

bobthekingofegypt commented Apr 3, 2016

highfive commented Apr 3, 2016

highfive commented Apr 3, 2016

This comment has been minimized.

KiChjang Apr 3, 2016

metajack commented Apr 4, 2016

metajack commented Apr 4, 2016

bobthekingofegypt commented Apr 4, 2016

metajack commented Apr 4, 2016

bors-servo commented Apr 4, 2016

metajack commented Apr 4, 2016

bobthekingofegypt commented Apr 4, 2016

bobthekingofegypt commented Apr 10, 2016

metajack commented Apr 13, 2016

perlun commented Apr 22, 2016

bobthekingofegypt commented Apr 23, 2016

bobthekingofegypt commented May 6, 2016

jdm commented Jun 8, 2016

metajack commented Jun 8, 2016

highfive commented Jun 10, 2016

jdm commented Jun 10, 2016

bors-servo commented Jun 10, 2016

bors-servo commented Jun 10, 2016

bors-servo commented Jun 10, 2016