fix reconnect issue for nodejs - #2407
Conversation
|
Anybody who could doublecheck this patch? |
We have used this patch in production |
emmenlau
left a comment
emmenlau
left a comment
There was a problem hiding this comment.
Generally looks good, but I'm no expert in this. Also, the same change may be needed in other Javascript-implementations?
|
|
||
| // If closed by manual, emit close event and cancel reconnect process | ||
| if(this.forceClose) { | ||
| self.emit("close"); |
There was a problem hiding this comment.
This may be overly pedantic, but would it be better to first clear the timer and then emit close? In other words, move the line self.emit("close"); down below the if (this.retry_timer) { block?
There was a problem hiding this comment.
Correct, close event handler maybe cause panic.
I have updated this change
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. |
|
Generally looks good to me. Could you kindly rebase on latest master and push again? |
|
This issue is no longer stale. Thank you for your contributions. |
Merge from apache/thrift:master
…-nodejs-reconnect-issue
|
@emmenlau rebased on latest master and pushed |
3 participants
CHANGES:
forceClosevariable for indicatingcloseaction caseued by manual, and give up reconnecting