Network error handling #226
Conversation
Codecov Report
@@ Coverage Diff @@
## 5.x #226 +/- ##
=========================================
+ Coverage 98.24% 98.54% +0.3%
=========================================
Files 17 17
Lines 2050 2408 +358
Branches 590 714 +124
=========================================
+ Hits 2014 2373 +359
+ Misses 36 35 -1
Continue to review full report at Codecov.
|
|
missing description of what is the issue this PR fixes ? |
| }); | ||
|
|
||
| this.socket.on('connect', function() { | ||
| if (self.wasConnected) { |
There was a problem hiding this comment.
According to socket.io documentation, connect event is fired "upon a connection including a successful reconnection".
If this is the case, don't we have a double trigger with the on('reconnect',..) one?
There was a problem hiding this comment.
Nope, this is different cases:
- reconnection on
connectevent is used when proxy close connection - reconnection on
reconnectevent is used when socket hang out and come back again via socket io reconnection logic
There was a problem hiding this comment.
Well, I see that you intend to use them differently but this is not what the doc says. connect is fired each time a connection is made, including socket.io reconnections.
There was a problem hiding this comment.
Ok, so I removed reconnect listener
| self.retrying = false; | ||
| self.connect(autoReconnect, reconnectionDelay); | ||
| }, reconnectionDelay); | ||
| } |
There was a problem hiding this comment.
isn't this part supposed to be natively handled by socket.io reconnect options?
There was a problem hiding this comment.
I don't know why but reconnection from socketio does not work... (love socketio <3)
There was a problem hiding this comment.
Hum.. If this is the case, shouldn't we then just not use it at all?
If it happens to work as expected, I still see an double trigger issue btw the connect and reconnect handlers (latest never being fired).
stafyniaksacha
changed the title
stafyniaksacha
changed the title
|
@ballinette @benoitvidis sorry for the description, I haven't saved it ^^ |
src/Room.js
Outdated
| subscribeQuery = self.kuzzle.addHeaders(subscribeQuery, this.headers); | ||
| subscribeQuery = self.kuzzle.addHeaders(subscribeQuery, self.headers); | ||
|
|
||
| networkListener = function() { |
There was a problem hiding this comment.
I don't like this behaviour (too many "addListener" and "removeListener").
Better just add following lines into src/Kuzzle.js (at line 365) :
self.network.onConnectError(function (error) {
// (...)
disableAllSubscriptions.call(self);
// (...)
});
function disableAllSubscriptions() {
var self = this;
Object.keys(self.subscriptions).forEach(function (roomId) {
Object.keys(self.subscriptions[roomId]).forEach(function (subscriptionId) {
var subscription = self.subscriptions[roomId][subscriptionId];
subscription.subscribing = false;
});
});
}It should work, but without a complex listener management.
There was a problem hiding this comment.
Work like a charm !
| }); | ||
|
|
||
| this.socket.on('connect', function() { | ||
| if (self.wasConnected) { |
There was a problem hiding this comment.
Well, I see that you intend to use them differently but this is not what the doc says. connect is fired each time a connection is made, including socket.io reconnections.
| self.retrying = false; | ||
| self.connect(autoReconnect, reconnectionDelay); | ||
| }, reconnectionDelay); | ||
| } |
There was a problem hiding this comment.
Hum.. If this is the case, shouldn't we then just not use it at all?
If it happens to work as expected, I still see an double trigger issue btw the connect and reconnect handlers (latest never being fired).
Bugfixes