Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Defer close while upgrading a transport #324

Merged
merged 2 commits into from Sep 25, 2014

Conversation

Projects
None yet
4 participants
Contributor

nkzawa commented Jul 17, 2014

Automattic/socket.io#1668

Wait for upgrade to finish since a close packet can't be sent while pausing a transport.
I'm not so confident yet, but we need to add closing readyState for making clear the current state.

@rauchg rauchg and 1 other commented on an outdated diff Jul 17, 2014

lib/socket.js
@@ -536,6 +534,10 @@ Socket.prototype.flush = function () {
Socket.prototype.write =
Socket.prototype.send = function (msg, fn) {
+ if ('closing' == this.readyState || 'closed' == this.readyState) {
+ return;
+ }
+
@rauchg

rauchg Jul 17, 2014

Contributor

Do we have a similar check in sendPacket?

@nkzawa

nkzawa Jul 18, 2014

Contributor

I think we need it only for sendPacket.
I will fix it later.

Contributor

rauchg commented Jul 17, 2014

Needs tests

Contributor

nkzawa commented Jul 19, 2014

Done. Please check it, @guille .

rauchg added a commit that referenced this pull request Sep 25, 2014

Merge pull request #324 from nkzawa/patch-5
Defer close while upgrading a transport

@rauchg rauchg merged commit 0097a5e into socketio:master Sep 25, 2014

Contributor

rauchg commented Sep 25, 2014

Thanks as usual @nkzawa

Contributor

rase- commented Sep 25, 2014

Thanks a ton @nkzawa!

Contributor

nkzawa commented Sep 26, 2014

I didn't notice about data loss related to the following.

Automattic/engine.io#284

It looks this PR doesn't fix the problem.
Maybe we should wait the drain event before closing when writeBuffer exists?

[edit]:
Data loss happens when upgrading a transport on client side too.

Contributor

3rd-Eden commented Sep 26, 2014

The problem mentioned above is a server side problem therefor it's impossible that this pull request or any other client related changes will fix that specific bug.

Contributor

nkzawa commented Sep 26, 2014

@3rd-Eden yeah, but I meant data loss happens on client side too. I tested that on my local. Sorry for confusing.

Contributor

rauchg commented Sep 26, 2014

Maybe we should wait the drain event before closing when writeBuffer exists?

That sounds like a good strategy

Contributor

nkzawa commented Sep 27, 2014

@guille Thanks, I will create a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment