You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@thrift.apache.org by guptanishank <gi...@git.apache.org> on 2016/12/19 07:43:41 UTC
[GitHub] thrift pull request #1141: support for timeout in node http connection
GitHub user guptanishank opened a pull request:
https://github.com/apache/thrift/pull/1141
support for timeout in node http connection
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/guptanishank/thrift master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/thrift/pull/1141.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 #1141
----
commit 3856c61b60028c6bcbba97fdce5ec346335e6f0b
Author: nishank <ni...@myntra.com>
Date: 2016-12-19T07:39:41Z
added timeout fro nodejs http connection
commit 41513f28c563887380fba9c2d13ed5f6b27a7991
Author: guptanishank <ni...@gmail.com>
Date: 2016-12-19T07:42:02Z
Merge pull request #1 from guptanishank/ng_timeout
added timeout fro nodejs http connection
----
---
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 #1141: support for timeout in node http connection
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1141#discussion_r146879631
--- Diff: lib/nodejs/lib/thrift/http_connection.js ---
@@ -214,6 +214,18 @@ HttpConnection.prototype.write = function(data) {
var req = (self.https) ?
https.request(self.nodeOptions, self.responseCallback) :
http.request(self.nodeOptions, self.responseCallback);
+
+ //support for timeout
+ var timeout = self.nodeOptions.timeout;
+ if(timeout){
+ req.on('socket', function (socket) {
+ socket.setTimeout(timeout);
+ socket.on('timeout', function() {
+ req.abort();
+ });
+ });
+ }
--- End diff --
It looks like additional work is needed here; @guptanishank will you be moving this forward?
---
[GitHub] thrift pull request #1141: support for timeout in node http connection
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1141#discussion_r93389394
--- Diff: lib/nodejs/lib/thrift/http_connection.js ---
@@ -214,6 +214,18 @@ HttpConnection.prototype.write = function(data) {
var req = (self.https) ?
https.request(self.nodeOptions, self.responseCallback) :
http.request(self.nodeOptions, self.responseCallback);
+
+ //support for timeout
+ var timeout = self.nodeOptions.timeout;
+ if(timeout){
+ req.on('socket', function (socket) {
+ socket.setTimeout(timeout);
+ socket.on('timeout', function() {
+ req.abort();
+ });
+ });
+ }
--- End diff --
Makes sense.
For newer versions of Node, it can become potentially problematic to do redundant timeout handling.
Current Node implementation seems to be ending up with setting socket timeout, so it may not be a problem for now.
Do you have any idea how to check whether the option is supported ?
Another minor point: we can reduce one indent level by using req.setTimeout(timeout, func...) ?
---
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 issue #1141: support for timeout in node http connection
Posted by Jens-G <gi...@git.apache.org>.
Github user Jens-G commented on the issue:
https://github.com/apache/thrift/pull/1141
Thanks! Could you please file a JIRA ticket according to our [contrib guidelines](http://thrift.apache.org/docs/HowToContribute)?
@all: Anyone to comment on this?
---
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 #1141: support for timeout in node http connection
Posted by guptanishank <gi...@git.apache.org>.
Github user guptanishank commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1141#discussion_r93419768
--- Diff: lib/nodejs/lib/thrift/http_connection.js ---
@@ -214,6 +214,18 @@ HttpConnection.prototype.write = function(data) {
var req = (self.https) ?
https.request(self.nodeOptions, self.responseCallback) :
http.request(self.nodeOptions, self.responseCallback);
+
+ //support for timeout
+ var timeout = self.nodeOptions.timeout;
+ if(timeout){
+ req.on('socket', function (socket) {
+ socket.setTimeout(timeout);
+ socket.on('timeout', function() {
+ req.abort();
+ });
+ });
+ }
--- End diff --
@nsuke One workaround could be to use a different option like "socketTimeout" and mention in documentation that this option is for node versions not having support for timeout.
Also, could you look patch submitted in this JIRA ticket: THRIFT-2968
---
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 #1141: support for timeout in node http connection
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1141#discussion_r93412832
--- Diff: lib/nodejs/lib/thrift/http_connection.js ---
@@ -214,6 +214,18 @@ HttpConnection.prototype.write = function(data) {
var req = (self.https) ?
https.request(self.nodeOptions, self.responseCallback) :
http.request(self.nodeOptions, self.responseCallback);
+
+ //support for timeout
+ var timeout = self.nodeOptions.timeout;
+ if(timeout){
+ req.on('socket', function (socket) {
+ socket.setTimeout(timeout);
+ socket.on('timeout', function() {
+ req.abort();
+ });
+ });
+ }
--- End diff --
Yes it turned out basically the same thing.
The potential problem was that, as long as we're using the name "timeout", Node will put their own timeout handler on newer versions.
It's especially nasty because a future version of Node can change it's internal behavior as to how exactly the timeout is handled, and it can cause subtle conflict with our own handling later.
But thinking about it, if it's what people have been widely doing, it's unlikely Node will start doing things that conflict with it.
---
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 issue #1141: support for timeout in node http connection
Posted by guptanishank <gi...@git.apache.org>.
Github user guptanishank commented on the issue:
https://github.com/apache/thrift/pull/1141
@Jens-G there is already a JIRA ticket related to this issue THRIFT-2968
---
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 #1141: support for timeout in node http connection
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1141#discussion_r93437696
--- Diff: lib/nodejs/lib/thrift/http_connection.js ---
@@ -214,6 +214,18 @@ HttpConnection.prototype.write = function(data) {
var req = (self.https) ?
https.request(self.nodeOptions, self.responseCallback) :
http.request(self.nodeOptions, self.responseCallback);
+
+ //support for timeout
+ var timeout = self.nodeOptions.timeout;
+ if(timeout){
+ req.on('socket', function (socket) {
+ socket.setTimeout(timeout);
+ socket.on('timeout', function() {
+ req.abort();
+ });
+ });
+ }
--- End diff --
I cannot see the content of the patch as it has been removed.
It's visible to everyone while it's available.
I slightly prefer the different-name idea.
As to the actual name, I would choose "requestTimeout" especially if we're going to invoke `request.setTimeout`.
Seeing the [code](https://github.com/nodejs/node/blob/master/lib/_http_client.js#L501), the timeout callback is only active during each request life-cycle, and removed after completion so that it does not affect HTTP keep-alive behavior.
In other words, direct `socket.setTimeout` would break socket reuse.
---
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 #1141: support for timeout in node http connection
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1141#discussion_r93366270
--- Diff: lib/nodejs/lib/thrift/http_connection.js ---
@@ -214,6 +214,18 @@ HttpConnection.prototype.write = function(data) {
var req = (self.https) ?
https.request(self.nodeOptions, self.responseCallback) :
http.request(self.nodeOptions, self.responseCallback);
+
+ //support for timeout
+ var timeout = self.nodeOptions.timeout;
+ if(timeout){
+ req.on('socket', function (socket) {
+ socket.setTimeout(timeout);
+ socket.on('timeout', function() {
+ req.abort();
+ });
+ });
+ }
--- End diff --
Reading the doc, simply putting 'timeout' key value to `nodeOptions` (without any code change) would activate request timeout.
`socket.setTimeout` is for idle connections which is very different thing.
So reusing the former value to the latter does not seem a good idea.
Do you need idle timeout for your use-case at all ?
---
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 #1141: support for timeout in node http connection
Posted by guptanishank <gi...@git.apache.org>.
Github user guptanishank commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1141#discussion_r93392404
--- Diff: lib/nodejs/lib/thrift/http_connection.js ---
@@ -214,6 +214,18 @@ HttpConnection.prototype.write = function(data) {
var req = (self.https) ?
https.request(self.nodeOptions, self.responseCallback) :
http.request(self.nodeOptions, self.responseCallback);
+
+ //support for timeout
+ var timeout = self.nodeOptions.timeout;
+ if(timeout){
+ req.on('socket', function (socket) {
+ socket.setTimeout(timeout);
+ socket.on('timeout', function() {
+ req.abort();
+ });
+ });
+ }
--- End diff --
Not sure on how to check whether the option is supported. I found out that node 4.x.x doesn't have it whereas node 6.x.x had it.
yes can reduce one indent level by using req.setTimeout
---
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 #1141: support for timeout in node http connection
Posted by nsuke <gi...@git.apache.org>.
Github user nsuke commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1141#discussion_r93458710
--- Diff: lib/nodejs/lib/thrift/http_connection.js ---
@@ -214,6 +214,18 @@ HttpConnection.prototype.write = function(data) {
var req = (self.https) ?
https.request(self.nodeOptions, self.responseCallback) :
http.request(self.nodeOptions, self.responseCallback);
+
+ //support for timeout
+ var timeout = self.nodeOptions.timeout;
+ if(timeout){
+ req.on('socket', function (socket) {
+ socket.setTimeout(timeout);
+ socket.on('timeout', function() {
+ req.abort();
+ });
+ });
+ }
--- End diff --
My understading is that the request one handles that, directly calling the socket one doesn't.
---
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 issue #1141: support for timeout in node http connection
Posted by bananer <gi...@git.apache.org>.
Github user bananer commented on the issue:
https://github.com/apache/thrift/pull/1141
@jeking3 it looks like a simple change now that support for node 4 can be dropped. However, I'm not sure what the expected behaviour in case of a timeout should be. Just from looking at the code it does not seem like error handling is triggered by the call to `req.abort()`…
---
[GitHub] thrift issue #1141: support for timeout in node http connection
Posted by jeking3 <gi...@git.apache.org>.
Github user jeking3 commented on the issue:
https://github.com/apache/thrift/pull/1141
@bananer this would be a good candidate to pick up and run with.
---
[GitHub] thrift pull request #1141: support for timeout in node http connection
Posted by guptanishank <gi...@git.apache.org>.
Github user guptanishank commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1141#discussion_r93457348
--- Diff: lib/nodejs/lib/thrift/http_connection.js ---
@@ -214,6 +214,18 @@ HttpConnection.prototype.write = function(data) {
var req = (self.https) ?
https.request(self.nodeOptions, self.responseCallback) :
http.request(self.nodeOptions, self.responseCallback);
+
+ //support for timeout
+ var timeout = self.nodeOptions.timeout;
+ if(timeout){
+ req.on('socket', function (socket) {
+ socket.setTimeout(timeout);
+ socket.on('timeout', function() {
+ req.abort();
+ });
+ });
+ }
--- End diff --
Doesn't setTimeout function handle that?
https://github.com/nodejs/node/blob/master/lib/_http_client.js#L625
---
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 #1141: support for timeout in node http connection
Posted by guptanishank <gi...@git.apache.org>.
Github user guptanishank commented on a diff in the pull request:
https://github.com/apache/thrift/pull/1141#discussion_r93375825
--- Diff: lib/nodejs/lib/thrift/http_connection.js ---
@@ -214,6 +214,18 @@ HttpConnection.prototype.write = function(data) {
var req = (self.https) ?
https.request(self.nodeOptions, self.responseCallback) :
http.request(self.nodeOptions, self.responseCallback);
+
+ //support for timeout
+ var timeout = self.nodeOptions.timeout;
+ if(timeout){
+ req.on('socket', function (socket) {
+ socket.setTimeout(timeout);
+ socket.on('timeout', function() {
+ req.abort();
+ });
+ });
+ }
--- End diff --
@nsuke the node version which i am using doesn't have support for timeout option: http://devdocs.io/node~4_lts/http.
---
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.
---