-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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 ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide some information about this PR (context, issue, what is the problem and what it solves?)
this.socket = window.io((this.ssl ? 'https://' : 'http://') + this.host + ':' + this.port, { | ||
reconnection: autoReconnect, | ||
reconnectionDelay: reconnectionDelay, | ||
forceNew: true | ||
}); | ||
|
||
this.socket.on('connect', function() { | ||
if (self.wasConnected) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, this is different cases:
- reconnection on
connect
event is used when proxy close connection - reconnection on
reconnect
event is used when socket hang out and come back again via socket io reconnection logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so I removed reconnect
listener
self.retrying = false; | ||
self.connect(autoReconnect, reconnectionDelay); | ||
}, reconnectionDelay); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't this part supposed to be natively handled by socket.io reconnect
options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why but reconnection from socketio does not work... (love socketio <3)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Work like a charm !
this.socket = window.io((this.ssl ? 'https://' : 'http://') + this.host + ':' + this.port, { | ||
reconnection: autoReconnect, | ||
reconnectionDelay: reconnectionDelay, | ||
forceNew: true | ||
}); | ||
|
||
this.socket.on('connect', function() { | ||
if (self.wasConnected) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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