UpdateTracter to reconnect and and re-subscribe on close events #30

jholleran · 2020-03-16T12:41:09Z

Resolves: #29

Kind of change

Proposed changes

Resubscribing to resources when Websocket connection closes. It will make 6 retry attempts with a exponential back-off time (1, 2, 4, 8, 16, 32 seconds) in between each attempt.

Also, as part of this PR it fixes an issue when unsubscribing from resources it will now allow the same resources to be fully subscribed again.

Relevant issues

Breaking change?

Yes
No

Requirements check

All tests are passing
New/updated tests are included

Additional info

First basic implementation of re-subscribing to resources when connection closes resubscribe with five attempts with 1 second backoff now with exponential backoff removed console log messages code style issues removed Increased to 6 attempts, also reset backoff times if connection succeeded Code style issue removed

coveralls · 2020-03-16T12:45:28Z

Pull Request Test Coverage Report for Build 163

19 of 19 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 152:	0.0%
Covered Lines:	291
Relevant Lines:	291

💛 - Coveralls

jholleran · 2020-03-16T13:23:57Z

src/UpdateTracker.js

        subscribers[url].delete(this.subscriber);
+        delete subscribers[url];
+        delete webSockets[new URL(url).host];


There is an issue here if there are multiple subscribed resources under the same domain. I'll change this to only delete if its the last resource with the domain.

RubenVerborgh

Hi @jholleran, thanks for the effort on this, especially the detailed unit tests.
The overall picture seems good, but I have some concerns here and there, which I have indicated with comments.

RubenVerborgh · 2020-03-16T13:17:28Z

.gitignore

@@ -3,3 +3,5 @@ dist
 lib
 module
 node_modules
+.idea/
+*.iml


Would suggest to keep those in a personal .gitignore.

Reverted this change.

RubenVerborgh · 2020-03-16T13:26:42Z

src/UpdateTracker.js

        subscribers[url].delete(this.subscriber);
+        delete subscribers[url];


The above will cause issues when there are multiple UpdateTracker instances. The idea is that subscribers[url] is shared across all UpdateTracker instances, representing a set of all subscribed UpdateTrackers. When unsubcribe is on a single instance, it should subscribe only that instance.

Reverted this change.

RubenVerborgh · 2020-03-16T13:28:06Z

src/UpdateTracker.js

        subscribers[url].delete(this.subscriber);
+        delete subscribers[url];
+        delete webSockets[new URL(url).host];


This seems to cause issues when an UpdateTracker is subscribed to two URLs from the same host, and we are only unsubscribing from one.

Yeah I agree this was incorrect and I've reverted it.

RubenVerborgh · 2020-03-16T13:29:35Z

src/UpdateTracker.js

@@ -57,11 +60,17 @@ function trackResource(url) {
  if (!webSocket) {
    const socketUrl = `${protocol.replace('http', 'ws')}//${host}/`;
    webSockets[host] = webSocket = new WebSocket(socketUrl);
+    const backOffDelay = delay || 1000;


Should we just call the function parameter backOffDelay instead of delay.
We can also initialize it as trackResource(url, retryAttempt = 0, backOffDelay = 1000).

Done. I've moved the default initialization to the setBackoff function as trackResource is called in a number of places already.

RubenVerborgh · 2020-03-16T13:31:17Z

src/UpdateTracker.js

  }

+  webSocket.resources.push(url); // Storing URL of resouce that will be subscribed to


Typo; can we put the comment above the line, and perhaps clarify that we keep track of resources so we can resubscribe if needed?

Also, might want to make webSocket.resources a Set to avoid multiple subscriptions.

RubenVerborgh · 2020-03-16T13:35:47Z

src/UpdateTracker.js

    Object.assign(webSocket, { enqueue, onmessage,
      ready: new Promise(resolve => (webSocket.onopen = resolve)),
+      onclose: oncloseFor(host),
+      resources: [],


Might want to make resources a Set to avoid multiple re-subscriptions.

RubenVerborgh · 2020-03-16T13:38:24Z

src/UpdateTracker.js

+function setUpBackOff(webSocket, retryAttempt, backOffDelay) {
+  Object.assign(webSocket, {
+    delay: backOffDelay,
+    backoff: new Promise(resolve => (setTimeout(resolve, backOffDelay))),


Given that backoff always seems to wait delay seconds, is it necessary to assign this as a function to the socket? Could we just do the waiting directly here https://github.com/solid/react-components/pull/30/files#diff-b2ffa2da88905c291381e2f58b66eb35R115 ?

Yes. This is a lot clearer moving it out of here.

RubenVerborgh · 2020-03-23T22:48:44Z

Hi @jholleran, I spent some time with the code, and simplified it down to 341ec20.

However, while doing so, I noticed this case: 12fce38
The reconnectAfterBackoff seems rely on the assumption that the socket on which it is called, is the active socket. However, that assumption fails.

I think this is an error in the tests, but wanted to verify with you.

jholleran · 2020-03-24T10:06:43Z

Yes, this is an error in the tests. If you run the two failing tests individually they will pass but when the full UpdateTracker-test suite runs they will fail.

The problem here is the webSocket array that is used in the test will hold the created WebSockets from the previous tests.

If the webSockets array is cleared before each test then there would be no WS to call the onclose on. So it was a bit of a hack to use the first WS in that array. Here the webSocket array is reset and will have the correct values for the rest of the test.

This was one thing that I struggled with writing them tests. It would be great to tidy this up a bit and have the test totally isolated. That was the reason that I was removing WS's on unsubscribe so that I could remove all and test on a clean slate (but this caused a bigger problem as I shouldn't have been deleting them).

RubenVerborgh · 2020-03-24T10:43:07Z

Got it, thanks. Will add a reset function for testing purposes.

RubenVerborgh · 2020-03-29T19:25:59Z

Thanks @jholleran!

RubenVerborgh self-assigned this Mar 16, 2020

RubenVerborgh added the enhancement New feature or request label Mar 16, 2020

jholleran commented Mar 16, 2020

View reviewed changes

RubenVerborgh suggested changes Mar 16, 2020

View reviewed changes

RubenVerborgh assigned jholleran and unassigned RubenVerborgh Mar 16, 2020

jholleran added 3 commits March 16, 2020 19:42

reverted removing WebSockets on unsubscribe

14cd227

moved backoff promise out of WS, tidied up initializing relay and retry

699412e

small tweak

8548707

RubenVerborgh force-pushed the feature/reconnect-on-close branch 2 times, most recently from 256c9de to 12fce38 Compare March 23, 2020 22:46

RubenVerborgh self-requested a review March 23, 2020 22:48

RubenVerborgh assigned RubenVerborgh and unassigned jholleran Mar 24, 2020

RubenVerborgh force-pushed the feature/reconnect-on-close branch from 12fce38 to 11de002 Compare March 29, 2020 18:39

Make socket tests independent.

927f1ce

RubenVerborgh force-pushed the feature/reconnect-on-close branch 2 times, most recently from 8333fb6 to b7e8cad Compare March 29, 2020 19:17

RubenVerborgh approved these changes Mar 29, 2020

View reviewed changes

RubenVerborgh force-pushed the feature/reconnect-on-close branch from b7e8cad to c67e3ff Compare March 29, 2020 19:22

Review socket code.

6ba0506

RubenVerborgh force-pushed the feature/reconnect-on-close branch from c67e3ff to 6ba0506 Compare March 29, 2020 19:25

RubenVerborgh merged commit e5685e4 into RubenVerborgh:master Mar 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UpdateTracter to reconnect and and re-subscribe on close events #30

UpdateTracter to reconnect and and re-subscribe on close events #30

jholleran commented Mar 16, 2020

coveralls commented Mar 16, 2020 •

edited

jholleran Mar 16, 2020

RubenVerborgh left a comment

RubenVerborgh Mar 16, 2020

jholleran Mar 16, 2020

RubenVerborgh Mar 16, 2020

jholleran Mar 16, 2020

RubenVerborgh Mar 16, 2020

jholleran Mar 16, 2020

RubenVerborgh Mar 16, 2020

jholleran Mar 16, 2020

RubenVerborgh Mar 16, 2020

jholleran Mar 16, 2020

RubenVerborgh Mar 16, 2020

jholleran Mar 16, 2020

RubenVerborgh Mar 16, 2020

jholleran Mar 16, 2020

RubenVerborgh commented Mar 23, 2020

jholleran commented Mar 24, 2020

RubenVerborgh commented Mar 24, 2020

RubenVerborgh commented Mar 29, 2020

		subscribers[url].delete(this.subscriber);
		delete subscribers[url];

		}

		webSocket.resources.push(url); // Storing URL of resouce that will be subscribed to

UpdateTracter to reconnect and and re-subscribe on close events #30

UpdateTracter to reconnect and and re-subscribe on close events #30

Conversation

jholleran commented Mar 16, 2020

Kind of change

Proposed changes

Relevant issues

Breaking change?

Requirements check

Additional info

coveralls commented Mar 16, 2020 • edited

Pull Request Test Coverage Report for Build 163

💛 - Coveralls

Choose a reason for hiding this comment

RubenVerborgh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RubenVerborgh commented Mar 23, 2020

jholleran commented Mar 24, 2020

RubenVerborgh commented Mar 24, 2020

RubenVerborgh commented Mar 29, 2020

coveralls commented Mar 16, 2020 •

edited