Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document error handling in http2 client connect example #16345

Closed
jsha opened this issue Oct 20, 2017 · 10 comments
Closed

Document error handling in http2 client connect example #16345

jsha opened this issue Oct 20, 2017 · 10 comments
Labels
http2 Issues or PRs related to the http2 subsystem.

Comments

@jsha
Copy link
Contributor

jsha commented Oct 20, 2017

  • Version: v8.7.0
  • Platform: Linux membrane 4.4.0-97-generic Embeddable concept #120-Ubuntu SMP Tue Sep 19 17:28:18 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
  • Subsystem: http2

Steps to reproduce:

Run the below program with node --expose_http2 sample.js

Expected result:

An error with ECONNREFUSED is thrown.

Actual result:

Program exits with no output.

var http2 = require('http2');
try {
  const client = http2.connect('https://localhost:12345');

  const req = client.request({ ':path': '/foo' });

  req.on('response', (headers) => {
    console.log(headers[':status']);
    console.log(headers['date']);
  });

  let data = '';
  req.setEncoding('utf8');
  req.on('data', (d) => data += d);
  req.on('end', () => client.destroy());
  req.on('error', (err) => console.error(err));
  req.end();
} catch(e) {
  console.error(e);
}
@mscdex mscdex added the http2 Issues or PRs related to the http2 subsystem. label Oct 20, 2017
@jasnell
Copy link
Member

jasnell commented Oct 20, 2017

that won't throw. Try adding a client.on('error', ...) handler

@jsha
Copy link
Contributor Author

jsha commented Oct 21, 2017 via email

@jasnell
Copy link
Member

jasnell commented Oct 21, 2017

No. The underlying code is specifically designed to handle connection errors asynchronously. Attaching the handler after should work just fine

@jsha
Copy link
Contributor Author

jsha commented Oct 21, 2017

I added a client.on('error', ...), but it doesn't appear to fire:

var http2 = require('http2');
const client = http2.connect('https://localhost:12345');
client.on('error', (err) => {
  console.error(err);
});

Output:

$ node --expose_http2 sample.js 
(node:4905) ExperimentalWarning: The http2 module is an experimental API.

@apapirovski
Copy link
Member

apapirovski commented Oct 21, 2017

Socket errors are actually emitted on socketError. So you would need to do client.on('socketError', (err) => console.log(err)); — I just double checked and it does log an ECONNREFUSED error.

Here's the documentation for it: https://nodejs.org/dist/latest-v8.x/docs/api/http2.html#http2_event_socketerror

@jasnell
Copy link
Member

jasnell commented Oct 21, 2017

Ha! Sigh that's right. Thank you for the correction @apapirovski!

@jsha jsha changed the title No exception thrown on http2 client connect failure Document error handling in http2 client connect example Oct 21, 2017
@jsha
Copy link
Contributor Author

jsha commented Oct 21, 2017

Thanks, that worked! I updated the issue title to suggest updating the example code for client usage to reflect handling error and socketError.

As general feedback: I spent a while reading the http2 documentation, and to me it wasn't clear which methods apply to client usage vs server usage. The impression I'm starting to come to is that the http2 API is much more "symmetric" than the http API, in that lots of the events apply to both sides. Is that accurate? If so, the documentation might benefit from explaining this up front, and/or documenting for each event whether it can be thrown by clients or servers.

Thanks,
Jacob

@apapirovski
Copy link
Member

I would say that's correct, yes — both the client and the server use the same basic building blocks in Http2Session & Http2Stream. The session is almost identical between client & server. There are a few more differences for the stream which you can see here:

https://nodejs.org/dist/latest-v8.x/docs/api/http2.html#http2_class_clienthttp2stream
https://nodejs.org/dist/latest-v8.x/docs/api/http2.html#http2_class_serverhttp2stream

In general, we are VERY happy to accept PRs that improve the docs. If you would like to add an example illustrating error handling or expand the existing example, that would be amazing!

@jsha
Copy link
Contributor Author

jsha commented Oct 21, 2017

Done: #16366.

Another bit of clarification: Is the compatibility API intended to include client-side code? I tried to port some existing code that used the https module, but it failed calling req.pipe(), because there was no pipe method available.

@apapirovski
Copy link
Member

The compatibility layer is strictly server side at the moment. For the client-side, I don't know if there's any plan to make it fully h1 compatible but — as far as I understand — we at the very least want to add something similar to the h1 Agent model.

I'm surprised pipe didn't work though. @ronag did a lot of work with http2 when building node-http2-proxy and I'm reasonably certain that's using pipe a bunch. See the source here https://github.com/nxtedition/node-http2-proxy/blob/master/index.js

addaleax pushed a commit to ayojs/ayo that referenced this issue Oct 26, 2017
Provide section headings for server-side and client side examples.
Add error handling and TLS to server-side example, following example
of `https`. Add error handling, TLS, more efficient Buffer usage,
and header printing to client example.

PR-URL: nodejs/node#16366
Fixes: nodejs/node#16345
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
gibfahn pushed a commit that referenced this issue Oct 30, 2017
Provide section headings for server-side and client side examples.
Add error handling and TLS to server-side example, following example
of `https`. Add error handling, TLS, more efficient Buffer usage,
and header printing to client example.

PR-URL: #16366
Fixes: #16345
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
gibfahn pushed a commit that referenced this issue Oct 30, 2017
Provide section headings for server-side and client side examples.
Add error handling and TLS to server-side example, following example
of `https`. Add error handling, TLS, more efficient Buffer usage,
and header printing to client example.

PR-URL: #16366
Fixes: #16345
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
gibfahn pushed a commit that referenced this issue Oct 31, 2017
Provide section headings for server-side and client side examples.
Add error handling and TLS to server-side example, following example
of `https`. Add error handling, TLS, more efficient Buffer usage,
and header printing to client example.

PR-URL: #16366
Fixes: #16345
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
addaleax pushed a commit to ayojs/ayo that referenced this issue Dec 7, 2017
Provide section headings for server-side and client side examples.
Add error handling and TLS to server-side example, following example
of `https`. Add error handling, TLS, more efficient Buffer usage,
and header printing to client example.

PR-URL: nodejs/node#16366
Fixes: nodejs/node#16345
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants