Always update connection state using _setState #175

gkubisa · 2017-10-17T15:19:57Z

Before this PR the connection event sequence could be like:

connected
disconnected
connected

when using a plain WebSocket and bindToSocket in a browser. If some kind of reconnecting socket were used (without bindToSocket), you might get the connecting events, too.

After this PR is merged, the connecting event would be emitted consistently regardless of the socket implementation, for example:

connecting
connected
disconnected
connecting
connected

coveralls · 2017-10-17T15:21:59Z

Coverage increased (+0.002%) to 96.108% when pulling 9548214 on Teamwork:connection-state-management into c0e2fee on share:master.

nateps · 2017-12-20T02:29:09Z

Thanks for highlighting this issue and proposing a solution! I agree that the inconsistency in events emitted when using bindToSocket() is an issue. The 'connecting' events should be emitted when transitioning from 'disconnected' to 'connected'.

A related but separate question is when constructing a ShareDB Connection, should a 'connecting' event be emitted immediately, and if you pass in a socket that is currently closed, should a 'disconnected' event be emitted immediately.

I'm inclined to maintain the current behavior, which is that no connection state events are emitted on construction. I think that instantiation triggering an event emission would be surprising. In addition, a typical use case for these events is showing a user a message to indicate that they are offline but the app is attempting to connect. Developers may or may not want to display the message on the initial connection, especially on page load. The preferred UX may be to show a connecting message only when the network unexpectedly had an issue and the client is attempting to reconnect for the user automatically. Emitting an event following construction would make this more complex to implement.

This may mean that developers have to take care to handle the initial connection state of the socket they are passing in when they initially create a Connection. This can be done by checking the connection.state or the socket.readyState. However, no additional check is needed in the case of synchronously creating a WebSocket and a Connection, since WebSockets are always created in the CONNECTING (0) readyState (https://html.spec.whatwg.org/multipage/web-sockets.html#the-websocket-interface:dom-websocket-readystate-2).

This means that the event sequence would typically be:

1. connected
2. disconnected
3. connecting
4. connected
etc

Or a repeated connection failure:

1. disconnected
2. connecting
3. disconnected
etc

I'll add inline comments for suggestions on the potential code changes.

Especially because this will change the behavior of the API in a subtle way, we'll also want to add unit tests before we merge this in. Started working on those with @rkstedman today: #188

nateps · 2017-12-20T02:49:30Z

lib/client/connection.js

@@ -48,6 +48,8 @@ function Connection(socket) {

  this.debug = false;

+  this.state = 'disconnected'


To avoid emission on initial construction, I'd set this based on the state of the socket passed in:

this.state = connectionState(socket);

And add the function:

function connectionState(socket) { if (socket.readyState === 0 || socket.readyState === 1) return 'connecting'; return 'disconnected'; }

nateps · 2017-12-20T02:52:49Z

lib/client/connection.js

@@ -90,7 +92,7 @@ Connection.prototype.bindToSocket = function(socket) {
  // - 'disconnected' Connection is closed, but it will reconnect automatically
  // - 'closed'       The connection was closed by the client, and will not reconnect
  // - 'stopped'      The connection was closed by the server, and will not reconnect
-  this.state = (socket.readyState === 0 || socket.readyState === 1) ? 'connecting' : 'disconnected';
+  this._setState((socket.readyState === 0 || socket.readyState === 1) ? 'connecting' : 'disconnected');


Update this line to use the function:

var newState = connectionState(socket); this._setState(newState);

coveralls · 2018-01-02T15:21:40Z

Coverage decreased (-0.04%) to 96.068% when pulling c2a5e13 on Teamwork:connection-state-management into c0e2fee on share:master.

coveralls · 2018-01-02T16:26:28Z

Coverage increased (+0.06%) to 96.161% when pulling 90043af on Teamwork:connection-state-management into c0e2fee on share:master.

coveralls · 2018-01-02T16:26:28Z

Coverage increased (+0.06%) to 96.161% when pulling 90043af on Teamwork:connection-state-management into c0e2fee on share:master.

gkubisa · 2018-01-02T17:13:31Z

I agree with your arguments about the synchronous initial event and made the changes you requested. As a side note, I think that it does not affect the observable behaviour because since the initial event was synchronous, there was no way to attach a listener to catch it.

I've also added some tests for the connection state.

nateps

LGTM!

Great contribution. Thanks so much! I'll merge and publish a new beta version.

nateps · 2018-01-20T00:39:28Z

test/client/connection.js

@@ -90,4 +91,71 @@ describe('client connection', function() {
    });
  });

+  describe('state management using setSocket', function() {
+    it('initial connection.state is connecting, if socket.readyState is CONNECTING', function () {
+        // https://html.spec.whatwg.org/multipage/web-sockets.html#dom-websocket-connecting


Love the links to the spec! 🥇

nateps · 2018-01-20T00:40:41Z

test/client/connection.js

+  describe('state management using setSocket', function() {
+    it('initial connection.state is connecting, if socket.readyState is CONNECTING', function () {
+        // https://html.spec.whatwg.org/multipage/web-sockets.html#dom-websocket-connecting
+        var socket = { readyState: 0 }


Style-wise, we tend not to put spaces inside of brackets, and use semicolons. I'll just go ahead and clean up after merging.

Always update connection state using _setState

9548214

nateps reviewed Dec 20, 2017

View reviewed changes

Avoid synchronous event when creating Connection

c2a5e13

rachael and others added 2 commits January 2, 2018 15:41

Add tests for connection state management

f1e3fa2

Add some connection tests

90043af

nateps approved these changes Jan 20, 2018

View reviewed changes

nateps merged commit 57fdd5c into share:master Jan 20, 2018

rkstedman mentioned this pull request Jan 20, 2018

Add state management tests for connection #188

Merged

gkubisa deleted the connection-state-management branch April 3, 2018 15:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Always update connection state using _setState #175

Always update connection state using _setState #175

Uh oh!

gkubisa commented Oct 17, 2017

Uh oh!

coveralls commented Oct 17, 2017 •

edited

Loading

Uh oh!

nateps commented Dec 20, 2017 •

edited

Loading

Uh oh!

nateps Dec 20, 2017

Uh oh!

nateps Dec 20, 2017

Uh oh!

coveralls commented Jan 2, 2018 •

edited

Loading

Uh oh!

coveralls commented Jan 2, 2018

Uh oh!

coveralls commented Jan 2, 2018 •

edited

Loading

Uh oh!

gkubisa commented Jan 2, 2018

Uh oh!

nateps left a comment

Uh oh!

nateps Jan 20, 2018

Uh oh!

nateps Jan 20, 2018

Uh oh!

Uh oh!

		@@ -48,6 +48,8 @@ function Connection(socket) {

		this.debug = false;

		this.state = 'disconnected'

Always update connection state using _setState #175

Always update connection state using _setState #175

Uh oh!

Conversation

gkubisa commented Oct 17, 2017

Uh oh!

coveralls commented Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nateps commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nateps Dec 20, 2017

Choose a reason for hiding this comment

Uh oh!

nateps Dec 20, 2017

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jan 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jan 2, 2018

Uh oh!

coveralls commented Jan 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gkubisa commented Jan 2, 2018

Uh oh!

nateps left a comment

Choose a reason for hiding this comment

Uh oh!

nateps Jan 20, 2018

Choose a reason for hiding this comment

Uh oh!

nateps Jan 20, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coveralls commented Oct 17, 2017 •

edited

Loading

nateps commented Dec 20, 2017 •

edited

Loading

coveralls commented Jan 2, 2018 •

edited

Loading

coveralls commented Jan 2, 2018 •

edited

Loading