Fix Issue #659 #682
Fix Issue #659 #682
Conversation
src/connection.js
Outdated
| @@ -689,17 +689,22 @@ class Connection extends EventEmitter { | |||
| if (err) { | |||
| return this.socketError(err); | |||
| } | |||
| if (this.state === this.STATE.FINAL) { | |||
| if (socket) { | |||
There was a problem hiding this comment.
I'm pretty sure socket is always set here.
Can we change the code to do an early return instead?
if (this.state === this.STATE.FINAL) {
socket.destroy();
return;
}
this.socket = socket;
...Also, could you add an empty newline before the if statement here?
|
This is great, thank you very much! I left a tiny comment regarding the code style, other than that, this is looking great! 👍 Can we also add an integration test for this, somehow? |
src/connection.js
Outdated
|
|
||
| this.socketConnect(); | ||
| if (this.state === this.STATE.FINAL) { | ||
| socket.destroy(); |
There was a problem hiding this comment.
The whitespace is still a bit off here. 😅
|
@arthurschreiber the pause-resume test has failed on Appveyor build. The test is flaky and fails when I test on a remote server, there's also a discussion about it in 518, could you please fix it? |
|
@Hadis-Fard I'll see what I can do. 😄 |
|
Perfect! Thanks for following up with the style changes. 😍 I'll merge and release this ASAP! |
|
This was just released as part of |
|
@arthurschreiber is there any chance we can get this back-ported to the 1.x branch for those of us stuck for the time being? We've started having servers die due to this recently, and we are not in a position to upgrade to v2 currently in some cases. |
|
If I remember correctly, the upgrade to v2 should be pretty painless. The only real change was dropping support for node versions earlier to 4. If possible I'd like to avoid spending time on backporting changes, but if you absolutely can not upgrade, I can go and do it. Please let me know! 😄 |
|
Thanks Arthur, If thats the main churn to v2, I can probably force it through our release process. I don't want to burden you with more releases in light of that. I appreciate the response! |
issue #659 : socket is assigned and attempted to connect after the connection has transitioned to final state by connect timeout.