Add reconnect support to eventbus (default off) with onreconnect event #660

yunyu · 2017-07-14T13:41:50Z

Adapted from https://stackoverflow.com/a/28497938. I've tested it in several scenarios and it seems to be reliable and doesn't cause stack overflows (tested with interval 10ms). The default behavior has no change from previous versions.

Reconnect is default off, enabled by calling reconnectEnabled(true)
Interval is 3000ms by default, can be set by passing vertxbus_reconnect_interval to options
onreconnect event is called after onopen event (just preference) and is fully optional, this makes it very easy to add reconnect support for most common use cases (tested on https://github.com/yunyu/SpeechDrop/blob/e22aaf2192bdaa054837b3ed53355725c9319cb0/frontend/js-orig/room.js#L44)
handlers and reply handlers are cleared on reconnect attempt, because I think it makes the most conceptual sense (both are tied to socket state) and it makes reconnection easily adoptable if handlers are registered onopen (see above example)
setupSockJSConnection is not exposed to the consumer, as improper use can lead to recursion issues and it saves bytes when uglified
there is no backoff currently implemented, I'm planning on it once some feedback is received
the reconnect is not triggered immediately (instead waits for interval) after disconnect, as there's a good chance that it won't work

Fixes vert-x3/issues#152

yunyu · 2017-07-14T13:51:31Z

Should this file used in tests be replaced as well? https://github.com/vert-x3/vertx-web/blob/12d681c8da166092ca24b09d7da9a83f8469e47f/vertx-sockjs-service-proxy/src/test/resources/vertx-js/vertx-eventbus.js

Also, I currently don't see any docs for pingEnabled (and so there's no obvious place to add them for reconnectEnabled). Thoughts about that?

pmlopes · 2017-07-18T14:14:11Z

vertx-sockjs-service-proxy/src/test/resources/vertx-js/vertx-eventbus.js

@@ -308,6 +340,16 @@
    }
  };

+  EventBus.prototype.reconnectEnabled = function (enable) {


i think the reverse name would be better: enableReconnect so it sound more natural when you read the code:
on the eventbus will set enable reconnect to true if you know what i mean...

I was thinking the same when I wrote this, but there's already an exposed method called pingEnabled which I thought to match.

I think we should rename that too.

Committed 36c177e, although this is a breaking change, I highly doubt that anybody has ever used the pingEnabled method previously (it's definitely not common)

Breaking changes are acceptable in 3.5.0 - as soon as they are listed.

pmlopes · 2017-07-18T14:16:08Z

@yunyu I think we should get rid of the test file and just use one all over the place, otherwise we're not testing it, right? and if you could also add a test for ping it would be great too!

yunyu · 2017-07-18T14:26:54Z

@pmlopes I'm not sure how to make the service proxy tests use the resource file from the main project 😞

The tests are currently structured to cover the basic things (sending messages over the bus), I'm not sure how ping tests could be added to the existing framework (since it's hard to test for a lack of something). I think I know how to add a test for reconnect, however.

yunyu · 2017-07-18T14:41:43Z

When running the tests, it seems like the service proxy copies the JS file over from vertx-web. I guess it's not that big of a problem if it's automatically replaced.

The only caveat is that the tests are run with the eventbus file before it's replaced, which will cause tests to fail the first time for anything that uses new/breaking features (but work the second time). I don't know enough about how the current POM is set up to fix that, unfortunately.

The easiest way to check is by first installing the vertx-web submodule under my branch into the local Maven repositiory (important), replacing the vertx-eventbus.js under the service proxy tests (also under my branch) with the previous/current one, and then running the tests for the service proxy twice. They will fail the first time and succeed the second time.

Also, currently calling EventBus.close() still makes the reconnect occur if it's enabled. Should that be the case? (Can't do an inline comment because it's not in the diff).

pmlopes · 2017-07-18T18:20:21Z

@cescoffier can you use your maven magic skills to assist @yunyu ?

cescoffier · 2017-07-20T16:40:53Z

@yunyu @pmlopes Gonna have a look tomorrow morning.

cescoffier · 2017-07-21T12:43:56Z

So, right now the vertx-eventbus.js used in the tests of the sockjs-service-proxy is pulled from the vertx-web project (so will only work if this one has been installed). I believe this is fine.

What's not great is to have this vertx-eventbus.js in the source tree of the sockjs-service-proxy. This can be easily fixed.

So this PR can be merged as it is, and I can get rid of the duplicated file.

yunyu · 2017-07-21T12:45:14Z

The problem seems to be that the file is copied after the tests are run, so the tests would fail the first time.

yunyu · 2017-07-21T14:00:17Z

This happens when you delete the file used in the service proxy, and run tests for the first time:

They will succeed the second time.

cescoffier · 2017-07-22T08:17:48Z

@yunyu Apply this to the project:

diff --git a/vertx-sockjs-service-proxy/pom.xml b/vertx-sockjs-service-proxy/pom.xml
index 3c4c794f..7ca31c89 100644
--- a/vertx-sockjs-service-proxy/pom.xml
+++ b/vertx-sockjs-service-proxy/pom.xml
@@ -134,7 +134,7 @@
             <configuration>
               <includeArtifactIds>vertx-web</includeArtifactIds>
               <includeClassifiers>client</includeClassifiers>
-              <outputDirectory>${project.basedir}/src/test/resources/vertx-js</outputDirectory>
+              <outputDirectory>${project.build.testOutputDirectory}/vertx-js</outputDirectory>
               <stripVersion>true</stripVersion>
             </configuration>
           </execution>
@@ -152,8 +152,8 @@
             <phase>process-test-resources</phase>
             <configuration>
               <target>
-                <move file="${project.basedir}/src/test/resources/vertx-js/vertx-web-client.js"
-                      tofile="${project.basedir}/src/test/resources/vertx-js/vertx-eventbus.js"/>
+                <move file="${project.build.testOutputDirectory}/vertx-js/vertx-web-client.js"
+                      tofile="${project.build.testOutputDirectory}/vertx-js/vertx-eventbus.js"/>
               </target>
             </configuration>
           </execution>

And remove src/test/resources/vertx-js - and you are done!

Notice that you must have installed the right version in your maven repo first (so mvn clean install from vertx-web).

yunyu · 2017-07-22T13:50:01Z

Cool, that worked! I assume it cleans out the test output dir after finishing the tests.

cescoffier · 2017-07-23T06:29:31Z

@yunyu @pmlopes it was just a missuse of the maven lifecycle and test classloading. Tests load the files from target/test-classes (that's their root). We were copying the event bus client into src/test/resources. We were assuming it would be copied into the test-classes directory. However, it was too late. The copy was bound to the process-test-resource phase. The copy is done in the previous phase. That's why it was working on the second run.

It was a build process bug.

yunyu · 2017-07-25T20:44:26Z

I adapted the backoff from https://github.com/mokesmokes/backo, which is what Socket.io is using. I think this is ready to merge at this point, the only thing missing is a ping test (which wasn't there earlier, and I'm not so sure if the lack of ping is easily testable).

@cescoffier

Also removes eventbus JS from service proxy tests per @cescoffier, renames pingEnabled to enablePing, adds enableReconnect method

onclose event is called after the max reconnect attempts are reached and the last attempt fails

cescoffier · 2017-08-01T07:35:22Z

@pmlopes WDYT ?

I think it can be part of the 3.5.0 Beta 1 so we can have feedback?

pmlopes · 2017-08-01T08:11:21Z

👍

yunyu · 2017-08-01T12:06:55Z

There should also be docs added sometime for the options. All of them have equivalents in Socket.io's manager script.

EmadAlblueshi · 2017-09-05T20:44:18Z

@yunyu Thanks!

vietj added the to review label Jul 14, 2017

pmlopes reviewed Jul 18, 2017

View reviewed changes

yunyu force-pushed the reconnect branch from 16c0d89 to 2c18740 Compare July 21, 2017 13:56

yunyu force-pushed the reconnect branch 2 times, most recently from bb8f352 to f934644 Compare July 22, 2017 18:01

yunyu force-pushed the reconnect branch from 987129e to a186efd Compare July 25, 2017 20:29

Yunyu Lin added 2 commits July 28, 2017 17:16

Add reconnect support to eventbus (default off) with onreconnect event

c0a3116

Also removes eventbus JS from service proxy tests per @cescoffier, renames pingEnabled to enablePing, adds enableReconnect method

Implement exponential backoff

d2882c6

yunyu force-pushed the reconnect branch from 3c70b31 to 8ea7e60 Compare July 28, 2017 21:16

Add configurable max reconnect attempts

8bb3fca

onclose event is called after the max reconnect attempts are reached and the last attempt fails

yunyu force-pushed the reconnect branch from 8ea7e60 to 8bb3fca Compare July 28, 2017 21:19

pmlopes merged commit 30240ae into vert-x3:master Aug 1, 2017

pmlopes removed the to review label Aug 1, 2017

tsegismont mentioned this pull request Sep 18, 2017

EventBus bridge doc says reconnection is not supported #696

Closed

davidmoshal mentioned this pull request Jul 5, 2018

websocket reconnect, can a server+client JS library be used? #964

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add reconnect support to eventbus (default off) with onreconnect event #660

Add reconnect support to eventbus (default off) with onreconnect event #660

yunyu commented Jul 14, 2017 •

edited

yunyu commented Jul 14, 2017 •

edited

pmlopes Jul 18, 2017

yunyu Jul 18, 2017

pmlopes Jul 18, 2017

yunyu Jul 18, 2017

cescoffier Jul 21, 2017

pmlopes commented Jul 18, 2017

yunyu commented Jul 18, 2017

yunyu commented Jul 18, 2017 •

edited

pmlopes commented Jul 18, 2017

cescoffier commented Jul 20, 2017

cescoffier commented Jul 21, 2017

yunyu commented Jul 21, 2017

yunyu commented Jul 21, 2017

cescoffier commented Jul 22, 2017

yunyu commented Jul 22, 2017

cescoffier commented Jul 23, 2017

yunyu commented Jul 25, 2017

cescoffier commented Aug 1, 2017 •

edited

pmlopes commented Aug 1, 2017

yunyu commented Aug 1, 2017

EmadAlblueshi commented Sep 5, 2017

Add reconnect support to eventbus (default off) with onreconnect event #660

Add reconnect support to eventbus (default off) with onreconnect event #660

Conversation

yunyu commented Jul 14, 2017 • edited

yunyu commented Jul 14, 2017 • edited

pmlopes Jul 18, 2017

Choose a reason for hiding this comment

yunyu Jul 18, 2017

Choose a reason for hiding this comment

pmlopes Jul 18, 2017

Choose a reason for hiding this comment

yunyu Jul 18, 2017

Choose a reason for hiding this comment

cescoffier Jul 21, 2017

Choose a reason for hiding this comment

pmlopes commented Jul 18, 2017

yunyu commented Jul 18, 2017

yunyu commented Jul 18, 2017 • edited

pmlopes commented Jul 18, 2017

cescoffier commented Jul 20, 2017

cescoffier commented Jul 21, 2017

yunyu commented Jul 21, 2017

yunyu commented Jul 21, 2017

cescoffier commented Jul 22, 2017

yunyu commented Jul 22, 2017

cescoffier commented Jul 23, 2017

yunyu commented Jul 25, 2017

cescoffier commented Aug 1, 2017 • edited

pmlopes commented Aug 1, 2017

yunyu commented Aug 1, 2017

EmadAlblueshi commented Sep 5, 2017

yunyu commented Jul 14, 2017 •

edited

yunyu commented Jul 14, 2017 •

edited

yunyu commented Jul 18, 2017 •

edited

cescoffier commented Aug 1, 2017 •

edited