Merge pull request #1941 from hypothesis/fix-reconnect-on-navigate

Fix spurious WebSocket disconnection errors

src/sidebar/test/websocket-test.js

-import Socket from '../websocket';
+import Socket, {
+  CLOSE_NORMAL,
+  CLOSE_GOING_AWAY,
+  CLOSE_ABNORMAL,
+} from '../websocket';
 
 describe('websocket wrapper', function() {
   let fakeSocket;
@@ -28,49 +32,57 @@ describe('websocket wrapper', function() {
     console.warn.restore();
   });
 
-  it('should reconnect after an abnormal disconnection', function() {
-    new Socket('ws://test:1234');
-    assert.ok(fakeSocket);
-    const initialSocket = fakeSocket;
-    fakeSocket.onopen({});
-    fakeSocket.onclose({ code: 1006 });
-    clock.tick(2000);
-    assert.ok(fakeSocket);
-    assert.notEqual(fakeSocket, initialSocket);
-  });
+  context('when the connection is closed by the browser or server', () => {
+    it('should reconnect after an abnormal disconnection', function() {
+      new Socket('ws://test:1234');
+      assert.ok(fakeSocket);
+      const initialSocket = fakeSocket;
+      fakeSocket.onopen({});
+      fakeSocket.onclose({ code: CLOSE_ABNORMAL });
+      clock.tick(2000);
+      assert.ok(fakeSocket);
+      assert.notEqual(fakeSocket, initialSocket);
+    });
 
-  it('should reconnect if initial connection fails', function() {
-    new Socket('ws://test:1234');
-    assert.ok(fakeSocket);
-    const initialSocket = fakeSocket;
-    fakeSocket.onopen({});
-    fakeSocket.onclose({ code: 1006 });
-    clock.tick(4000);
-    assert.ok(fakeSocket);
-    assert.notEqual(fakeSocket, initialSocket);
-  });
+    it('should reconnect if initial connection fails', function() {
+      new Socket('ws://test:1234');
+      assert.ok(fakeSocket);
+      const initialSocket = fakeSocket;
+      fakeSocket.onopen({});
+      fakeSocket.onclose({ code: CLOSE_ABNORMAL });
+      clock.tick(4000);
+      assert.ok(fakeSocket);
+      assert.notEqual(fakeSocket, initialSocket);
+    });
 
-  it('should send queued messages after a reconnect', function() {
-    // simulate WebSocket setup and initial connection
-    const socket = new Socket('ws://test:1234');
-    fakeSocket.onopen({});
+    it('should send queued messages after a reconnect', function() {
+      // simulate WebSocket setup and initial connection
+      const socket = new Socket('ws://test:1234');
+      fakeSocket.onopen({});
 
-    // simulate abnormal disconnection
-    fakeSocket.onclose({ code: 1006 });
+      // simulate abnormal disconnection
+      fakeSocket.onclose({ code: CLOSE_ABNORMAL });
 
-    // enqueue a message and check that it is sent after the WS reconnects
-    socket.send({ aKey: 'aValue' });
-    fakeSocket.onopen({});
-    assert.calledWith(fakeSocket.send, '{"aKey":"aValue"}');
-  });
+      // enqueue a message and check that it is sent after the WS reconnects
+      socket.send({ aKey: 'aValue' });
+      fakeSocket.onopen({});
+      assert.calledWith(fakeSocket.send, '{"aKey":"aValue"}');
+    });
 
-  it('should not reconnect after a normal disconnection', function() {
-    const socket = new Socket('ws://test:1234');
-    socket.close();
-    assert.called(fakeSocket.close);
-    const initialSocket = fakeSocket;
-    clock.tick(2000);
-    assert.equal(fakeSocket, initialSocket);
+    [CLOSE_NORMAL, CLOSE_GOING_AWAY].forEach(closeCode => {
+      it('should not reconnect after a normal disconnection', function() {
+        new Socket('ws://test:1234');
+        assert.ok(fakeSocket);
+        const initialSocket = fakeSocket;
+
+        fakeSocket.onopen({});
+        fakeSocket.onclose({ code: closeCode });
+        clock.tick(4000);
+
+        assert.ok(fakeSocket);
+        assert.equal(fakeSocket, initialSocket);
+      });
+    });
   });
 
   it('should queue messages sent prior to connection', function() {
@@ -87,4 +99,22 @@ describe('websocket wrapper', function() {
     socket.send({ abc: 'foo' });
     assert.calledWith(fakeSocket.send, '{"abc":"foo"}');
   });
+
+  describe('#close', () => {
+    it('should close the socket with a normal status', () => {
+      const socket = new Socket('ws://test:1234');
+      socket.close();
+      assert.calledWith(fakeSocket.close, CLOSE_NORMAL);
+    });
+
+    it('should not reconnect after closing', () => {
+      const socket = new Socket('ws://test:1234');
+      const initialSocket = fakeSocket;
+
+      socket.close();
+
+      clock.tick(2000);
+      assert.equal(fakeSocket, initialSocket);
+    });
+  });
 });

src/sidebar/websocket.js

 import retry from 'retry';
 import EventEmitter from 'tiny-emitter';
 
-// see https://developer.mozilla.org/en-US/docs/Web/API/CloseEvent
-const CLOSE_NORMAL = 1000;
+// Status codes indicating the reason why a WebSocket connection closed.
+// See https://developer.mozilla.org/en-US/docs/Web/API/CloseEvent and
+// https://tools.ietf.org/html/rfc6455#section-7.4.
+
+// "Normal" closures.
+export const CLOSE_NORMAL = 1000;
+export const CLOSE_GOING_AWAY = 1001;
+
+// "Abnormal" closures.
+export const CLOSE_ABNORMAL = 1006;
+
+// There are other possible close status codes not listed here. They are all
+// considered abnormal closures.
 
 // Minimum delay, in ms, before reconnecting after an abnormal connection close.
 const RECONNECT_MIN_DELAY = 1000;
@@ -60,7 +71,7 @@ export default class Socket extends EventEmitter {
           self.emit('open', event);
         };
         socket.onclose = function(event) {
-          if (event.code === CLOSE_NORMAL) {
+          if (event.code === CLOSE_NORMAL || event.code === CLOSE_GOING_AWAY) {
             self.emit('close', event);
             return;
           }
@@ -109,7 +120,20 @@ export default class Socket extends EventEmitter {
 
     /** Close the underlying WebSocket connection */
     this.close = function() {
-      socket.close();
+      // nb. Always sent a status code in the `close()` call to work around
+      // a problem in the backend's ws4py library.
+      //
+      // If no status code is provided in the `close()` call, the browser will
+      // send a close frame with no payload, which is allowed by the spec.
+      // ws4py however, will respond by sending back a close frame with a 1005
+      // status code, which is not allowed by the spec. What ws4py should do in
+      // that scenario is send back a close frame with no payload itself. This
+      // invalid close frame causes browsers to report an abnormal WS
+      // termination, even though nothing really went wrong.
+      //
+      // To avoid the problem, we just explicitly send a "closed normally"
+      // status code here and ws4py will respond with the same status.
+      socket.close(CLOSE_NORMAL);
     };
 
     /**