http: allow async createConnection() #4638

mscdex · 2016-01-12T04:49:53Z

This commit adds support for async createConnection() implementations and is still backwards compatible with synchronous createConnection() implementations.

This commit also makes the http client more friendly with generic stream objects produced by createConnection() by checking stream.writable instead of stream.destroyed as the latter is currently a net.Socket-ism and not set by the core stream implementations.

The original use case behind this PR is that it allows the creation of a minimal, custom Http.Agent that allows you to make in-process http requests over ssh. The async-ness comes into play because the way ssh works is you have to ask the server to start a TCP connection first. You could return some sort of dummy socket object, but ssh2 already provides a stream object when the server allows the TCP connection, so adding yet another object for every connection just makes things more complicated.

Note: I wasn't quite sure how to handle the errors passed to the oncreate() callback. The same problem exists for the synchronous use. How do you signal to the http client/agent that no socket was able to be created without returning a dummy object that emits an error or something?

mscdex · 2016-01-20T18:14:26Z

/cc @nodejs/collaborators Any comments/input on this?

mcollina · 2016-01-20T19:05:45Z

It's 👍 for me, because the alternative way of doing this is to return a 'dummy' duplex, possibly built with duplexify that proxies to the real one.

However, I would add an example on why it is needed here and in the docs, otherwise is not exactly clear the case when this is useful.

Regarding errors, you should probably emit them in the req, as it is done here:

node/lib/_http_client.js

Line 262 in bbaf0f9

req.emit('error', err);

trevnorris · 2016-01-20T19:08:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/1322/

indutny · 2016-01-20T21:02:57Z

lib/_http_agent.js

+  var called = false;
+  var newSocket = self.createConnection(options);
+  if (newSocket)
+    oncreate(null, newSocket);


process.nextTick to be consistent?

You mean put a process.nextTick() inside oncreate() that wraps the majority of that code? Otherwise oncreate() could be executed immediately within createConnection() too....

Ah, ok... makes sense!

To me it should still fit with the old behavior as it is now without process.nextTick().

mscdex · 2016-01-20T21:12:49Z

@mcollina The problem about errors is that there are some places where a request (or other suitable) object is not (yet) available when createConnection() is called. An example of this is inside http.Agent.prototype.createSocket(). Where would errors go in such cases?

Maybe adding error trapping and such would be better done in a separate PR?

mcollina · 2016-01-20T22:05:55Z

@mscdex maybe I missing some bits, but isn't createSocket() accepting a request as a first argument? https://github.com/nodejs/node/blob/master/lib/_http_agent.js#L152.

I think error handling for this should be done as part of this PR, because the current API has a clear error model, while this swallow errors in the callback.

mcollina · 2016-01-20T22:06:44Z

lib/_http_agent.js

-    req.onSocket(this.createSocket(req, options));
+    this.createSocket(req, options, function(err, newSocket) {
+      if (!err)
+        req.onSocket(newSocket);


@mscdex I mean adding req.emit('error', err) as an else branch here.

mscdex · 2016-01-20T22:12:12Z

@mcollina If you look a few lines below that you'll see that there is an if (req) {} check.

EDIT: Perhaps that conditional should be removed. I will look into that as well.

mcollina · 2016-01-20T22:35:03Z

@mscdex I think that conditional should be removed as well. I don't see any point around where req can be null there. createSocket is only called by addRequest, and addRequest is only called from a single place:

node/lib/_http_client.js

Line 137 in bbaf0f9

self.agent.addRequest(self, options);

.

mscdex · 2016-01-21T00:54:59Z

PR updated according to suggestions

mscdex · 2016-01-21T00:55:41Z

CI again: https://ci.nodejs.org/job/node-test-pull-request/1334/

indutny · 2016-01-21T01:15:17Z

lib/_http_agent.js

+  if (newSocket)
+    oncreate(null, newSocket);
+  function oncreate(err, s) {
+    if (called)


Should we throw an error instead?

Probably not, there wouldn't be any reliable way to catch such errors since this is done inside the agent and createSocket() could be called whenever it needs a new socket?

mscdex · 2016-01-31T21:44:38Z

CI is green except for some unrelated test failures on pi1.

jasnell · 2016-02-01T17:21:54Z

Needs a doc update too ... otherwise LGTM

mscdex · 2016-02-01T17:31:16Z

@jasnell The createConnection isn't documented already so there's nothing to update, unless you're proposing to add it to the documentation.

jasnell · 2016-02-01T17:42:36Z

Yeah, that's what I'm proposing :-)

mcollina · 2016-02-02T14:09:45Z

LGTM.

I agree with @jasnell, we need to add this to the docs, it can be quite handy.
Also, it would become part of the API and it will need to adhere to semver.

mscdex · 2016-02-04T03:18:29Z

@jasnell @mcollina Documentation added.

mcollina · 2016-02-04T08:51:19Z

LGTM

jasnell · 2016-02-04T18:05:43Z

LGTM

jasnell · 2016-02-04T18:06:12Z

New CI, just to be safe: https://ci.nodejs.org/job/node-test-pull-request/1546/

This commit adds support for async createConnection() implementations and is still backwards compatible with synchronous createConnection() implementations. This commit also makes the http client more friendly with generic stream objects produced by createConnection() by checking stream.writable instead of stream.destroyed as the latter is currently a net.Socket-ism and not set by the core stream implementations.

mscdex · 2016-02-04T19:38:59Z

Fixed lint issue. CI again: https://ci.nodejs.org/job/node-test-pull-request/1551/

This commit adds support for async createConnection() implementations and is still backwards compatible with synchronous createConnection() implementations. This commit also makes the http client more friendly with generic stream objects produced by createConnection() by checking stream.writable instead of stream.destroyed as the latter is currently a net.Socket-ism and not set by the core stream implementations. PR-URL: #4638 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

mscdex · 2016-02-11T17:57:19Z

Landed in 9bee03a.

This commit adds support for async createConnection() implementations and is still backwards compatible with synchronous createConnection() implementations. This commit also makes the http client more friendly with generic stream objects produced by createConnection() by checking stream.writable instead of stream.destroyed as the latter is currently a net.Socket-ism and not set by the core stream implementations. PR-URL: #4638 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

This commit adds support for async createConnection() implementations and is still backwards compatible with synchronous createConnection() implementations. This commit also makes the http client more friendly with generic stream objects produced by createConnection() by checking stream.writable instead of stream.destroyed as the latter is currently a net.Socket-ism and not set by the core stream implementations. PR-URL: nodejs#4638 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: James M Snell <[email protected]>

lekkas · 2016-03-03T18:41:29Z

Sweet, allowing async createConnection() will facilitate creating tunnelling sockets. Will this get merged in the 4.x branch?

mscdex · 2016-03-03T18:53:21Z

@lekkas That would be up to @nodejs/lts

lekkas · 2016-03-03T19:37:44Z

Thank you @mscdex and great work btw 👍

jasnell · 2016-03-03T22:06:11Z

As a semver-minor it's not likely. So far the only semver-minor's we have added to LTS deal with lower level post mortem debugging support. I doubt there'd be enough justification on this one.

lekkas · 2016-03-07T13:56:43Z

Got it, thanks @jasnell

mscdex added the http Issues or PRs related to the http subsystem. label Jan 12, 2016

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 12, 2016

indutny reviewed Jan 20, 2016
View reviewed changes

mcollina reviewed Jan 20, 2016
View reviewed changes

mscdex force-pushed the http-async-createconnection branch from abf0619 to d245599 Compare January 21, 2016 00:51

indutny reviewed Jan 21, 2016
View reviewed changes

mscdex force-pushed the http-async-createconnection branch from d245599 to 0b04074 Compare February 4, 2016 03:17

mscdex force-pushed the http-async-createconnection branch from 0b04074 to 2ac8fc8 Compare February 4, 2016 19:24

mscdex force-pushed the http-async-createconnection branch from 2ac8fc8 to 4518b3a Compare February 4, 2016 19:24

mscdex closed this Feb 11, 2016

mscdex deleted the http-async-createconnection branch February 11, 2016 17:56

This was referenced Mar 10, 2016

fix: should handle socket error node-modules/agentkeepalive#29

Closed

async createCollection() node-modules/agentkeepalive#31

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: allow async createConnection() #4638

http: allow async createConnection() #4638

mscdex commented Jan 12, 2016

mscdex commented Jan 20, 2016

mcollina commented Jan 20, 2016

trevnorris commented Jan 20, 2016

indutny Jan 20, 2016

mscdex Jan 20, 2016

indutny Jan 20, 2016

mscdex Jan 20, 2016

mscdex commented Jan 20, 2016

mcollina commented Jan 20, 2016

mcollina Jan 20, 2016

mscdex commented Jan 20, 2016

mcollina commented Jan 20, 2016

mscdex commented Jan 21, 2016

mscdex commented Jan 21, 2016

indutny Jan 21, 2016

mscdex Jan 21, 2016

mscdex commented Jan 31, 2016

jasnell commented Feb 1, 2016

mscdex commented Feb 1, 2016

jasnell commented Feb 1, 2016

mcollina commented Feb 2, 2016

mscdex commented Feb 4, 2016

mcollina commented Feb 4, 2016

jasnell commented Feb 4, 2016

jasnell commented Feb 4, 2016

mscdex commented Feb 4, 2016

mscdex commented Feb 11, 2016

lekkas commented Mar 3, 2016

mscdex commented Mar 3, 2016

lekkas commented Mar 3, 2016

jasnell commented Mar 3, 2016

lekkas commented Mar 7, 2016

http: allow async createConnection() #4638

http: allow async createConnection() #4638

Conversation

mscdex commented Jan 12, 2016

mscdex commented Jan 20, 2016

mcollina commented Jan 20, 2016

trevnorris commented Jan 20, 2016

indutny Jan 20, 2016

Choose a reason for hiding this comment

mscdex Jan 20, 2016

Choose a reason for hiding this comment

indutny Jan 20, 2016

Choose a reason for hiding this comment

mscdex Jan 20, 2016

Choose a reason for hiding this comment

mscdex commented Jan 20, 2016

mcollina commented Jan 20, 2016

mcollina Jan 20, 2016

Choose a reason for hiding this comment

mscdex commented Jan 20, 2016

mcollina commented Jan 20, 2016

mscdex commented Jan 21, 2016

mscdex commented Jan 21, 2016

indutny Jan 21, 2016

Choose a reason for hiding this comment

mscdex Jan 21, 2016

Choose a reason for hiding this comment

mscdex commented Jan 31, 2016

jasnell commented Feb 1, 2016

mscdex commented Feb 1, 2016

jasnell commented Feb 1, 2016

mcollina commented Feb 2, 2016

mscdex commented Feb 4, 2016

mcollina commented Feb 4, 2016

jasnell commented Feb 4, 2016

jasnell commented Feb 4, 2016

mscdex commented Feb 4, 2016

mscdex commented Feb 11, 2016

lekkas commented Mar 3, 2016

mscdex commented Mar 3, 2016

lekkas commented Mar 3, 2016

jasnell commented Mar 3, 2016

lekkas commented Mar 7, 2016