fix reconnect issue for nodejs #2407
Conversation
|
Anybody who could doublecheck this patch? |
We have used this patch in production |
lib/nodejs/lib/thrift/connection.js
Outdated
| @@ -198,6 +200,16 @@ Connection.prototype.connection_gone = function () { | |||
| var self = this; | |||
| this.connected = false; | |||
|
|
|||
| // 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 |
CHANGES:
forceClosevariable for indicatingcloseaction caseued by manual, and give up reconnecting