You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by jamesreggio <gi...@git.apache.org> on 2016/04/14 01:44:00 UTC
[GitHub] thrift pull request: THRIFT-3787 Fix Node.js Connection object err...
GitHub user jamesreggio opened a pull request:
https://github.com/apache/thrift/pull/986
THRIFT-3787 Fix Node.js Connection object error handling
The `connected` property on `Connection` instances was not accurately maintained if reconnection retries are not enabled. Line 194 was moved to Line 176 to fix this.
Furthermore, reconnection retries are not possible with secure sockets, so this commit returns early in that case, preventing long delays waiting for the `secureConnect` event which will never fire again. It's not explicitly stated in the Node.js documentation, but calling `connect()` on an instance of a `TLSSocket` will only reconnect the underlying TCP connection; it is not possible to cause the socket to re-handshake at the TLS layer.
(You can peruse the source for [`tls.connect`](https://github.com/nodejs/node/blob/a11d506decd2820221d6cd770990a585ca0261d5/lib/_tls_wrap.js#L964-1095) to see that it performs many more operations than just instantiating a `TLSSocket` instance and calling `connect()` on it. This stands in contrast to an ordinary TCP `Socket` instance from the `net` module, where reconnection is possible via these means.)
Supporting reconnection for secure sockets would require a deep refactor to enable the Thrift `Connection` instance to replace its underlying socket, and I didn't feel up to the challenge today.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jamesreggio/thrift THRIFT-3787
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/986.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #986
----
commit 449b1d711f91a9252b64351a71e44945e4432911
Author: James Reggio <ja...@gmail.com>
Date: 2016-04-13T23:33:40Z
THRIFT-3787 Fix Node.js Connection object error handling
The `connected` property on a Connection instances was not accurately
maintained if reconnection retries are not enabled.
Furthermore, reconnection retries are not possible with secure sockets,
so this commit returns early in that case, preventing long delays.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3787 Fix Node.js Connection object err...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/thrift/pull/986
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3787 Fix Node.js Connection object err...
Posted by jamesreggio <gi...@git.apache.org>.
Github user jamesreggio commented on a diff in the pull request:
https://github.com/apache/thrift/pull/986#discussion_r59645180
--- Diff: lib/nodejs/lib/thrift/connection.js ---
@@ -81,15 +81,7 @@ var Connection = exports.Connection = function(stream, options) {
});
this.connection.addListener("error", function(err) {
- // Only emit the error if no-one else is listening on the connection
- // or if someone is listening on us
- if (self.connection.listeners('error').length === 1 ||
- self.listeners('error').length > 0) {
- self.emit("error", err);
- }
--- End diff --
This code doesn't make any sense; emitting `error` with no listeners is a no-op, so we might as well do it always.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3787 Fix Node.js Connection object err...
Posted by jamesreggio <gi...@git.apache.org>.
Github user jamesreggio commented on a diff in the pull request:
https://github.com/apache/thrift/pull/986#discussion_r59645268
--- Diff: lib/nodejs/lib/thrift/connection.js ---
@@ -181,19 +173,18 @@ Connection.prototype.write = function(data) {
Connection.prototype.connection_gone = function () {
var self = this;
+ this.connected = false;
// If a retry is already in progress, just let that happen
if (this.retry_timer) {
return;
}
- if (!this.max_attempts) {
+ // We cannot reconnect a secure socket.
+ if (!this.max_attempts || this.ssl) {
self.emit("close");
return;
}
- this.connected = false;
- this.ready = false;
--- End diff --
`ready` was unused.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3787 Fix Node.js Connection object err...
Posted by jamesreggio <gi...@git.apache.org>.
Github user jamesreggio commented on the pull request:
https://github.com/apache/thrift/pull/986#issuecomment-209693707
Somebody please kick Jenkins — it's repo instance is in a bad state.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3787 Fix Node.js Connection object err...
Posted by jamesreggio <gi...@git.apache.org>.
Github user jamesreggio commented on a diff in the pull request:
https://github.com/apache/thrift/pull/986#discussion_r59645316
--- Diff: lib/nodejs/lib/thrift/connection.js ---
@@ -181,19 +173,18 @@ Connection.prototype.write = function(data) {
Connection.prototype.connection_gone = function () {
var self = this;
+ this.connected = false;
// If a retry is already in progress, just let that happen
if (this.retry_timer) {
return;
}
- if (!this.max_attempts) {
+ // We cannot reconnect a secure socket.
+ if (!this.max_attempts || this.ssl) {
self.emit("close");
return;
}
- this.connected = false;
--- End diff --
Prior to this change, this would not be run if `this.max_attempts` was unset.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3787 Fix Node.js Connection object err...
Posted by jamesreggio <gi...@git.apache.org>.
Github user jamesreggio commented on a diff in the pull request:
https://github.com/apache/thrift/pull/986#discussion_r59645250
--- Diff: lib/nodejs/lib/thrift/connection.js ---
@@ -81,15 +81,7 @@ var Connection = exports.Connection = function(stream, options) {
});
this.connection.addListener("error", function(err) {
- // Only emit the error if no-one else is listening on the connection
- // or if someone is listening on us
- if (self.connection.listeners('error').length === 1 ||
- self.listeners('error').length > 0) {
- self.emit("error", err);
- }
- // "error" events get turned into exceptions if they aren't listened for. If the user handled this error
- // then we should try to reconnect.
- self.connection_gone();
--- End diff --
Per [Node.js documentation](https://nodejs.org/api/net.html#net_event_error_1), `error` is always followed by `close`, so there's no need to call `connection_gone()` twice.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] thrift pull request: THRIFT-3787 Fix Node.js Connection object err...
Posted by jamesreggio <gi...@git.apache.org>.
Github user jamesreggio commented on the pull request:
https://github.com/apache/thrift/pull/986#issuecomment-218791344
Ping, @RandyAbernethy. I removed the hunk relating to the `error` event, per your comments in JIRA.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---