Skip to content
This repository has been archived by the owner on Feb 11, 2020. It is now read-only.

Prevent FIN_WAIT connections by using stream.destorySoon() #524

Merged
merged 2 commits into from Jul 24, 2016
Merged

Prevent FIN_WAIT connections by using stream.destorySoon() #524

merged 2 commits into from Jul 24, 2016

Conversation

eladnava
Copy link
Contributor

As per #306.

if (that.connection.stream.destroySoon) {
that.connection.stream.destroySoon();
}
else {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please move the else inline with the }?

@mcollina
Copy link
Collaborator

I'm thinking we can have a three way case: destroySoon, destroy and end. In this order.

What do you think?

@eladnava
Copy link
Contributor Author

@mcollina Done, however I wasn't able to run the tests completely as I don't have Redis installed.

@mcollina
Copy link
Collaborator

@behard what do you think?

@eladnava
Copy link
Contributor Author

I think you meant @behrad =)

@mcollina
Copy link
Collaborator

Oh yes :D.

@behrad
Copy link
Contributor

behrad commented Jul 19, 2016

So it should reduce FIN_WAITs :)
If destroySoon is preferable, how will that affect on the mobile/public facing client end?

@eladnava
Copy link
Contributor Author

@behrad When connected clients passively disconnect from the internet, they will eventually be kicked due to Mosca's keepalive timeout mechanism, which calls socket.end() if the client hasn't sent an MQTT keep alive packet within the client-defined time limit.

In its current implementation, calling socket.end() on those connections will have them stuck (if not indefinitely, then for a long time) in FIN_WAIT states because the TCP stack will attempt to let the client know it's closing down the connection. But since the client is no longer reachable then the server will never be able to deliver the FIN/ACK packet to the client, nor will it receive a FIN/ACK or an ACK from the client in response to the FIN/ACK.

ssldig09

Does that answer your question?

@behrad
Copy link
Contributor

behrad commented Jul 19, 2016

great explanation @eladnava 👍
I can test to see how this patch will change the real world scenario :)

@eladnava
Copy link
Contributor Author

@behrad excellent, please let us know if it's OK to merge :)

@mcollina
Copy link
Collaborator

Ping @behrad? I'll like to merge this and release next week!

@behrad
Copy link
Contributor

behrad commented Jul 24, 2016

Sorry guys, so busy these days, it takes me some time to test this, I'l let you even after @mcollina releases this :)

@mcollina mcollina merged commit c22d564 into moscajs:master Jul 24, 2016
@eladnava eladnava deleted the stream-destroysoon branch July 24, 2016 09:49
@eladnava
Copy link
Contributor Author

@mcollina Has this been released yet? I'd love to use it in production. Thanks! 😄

@mcollina
Copy link
Collaborator

Sorry, released now as v2.1.0.

@eladnava
Copy link
Contributor Author

Excellent, thank you! 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants