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

test,doc: address http2 connection abort/network error handling #21861

Closed
wants to merge 4 commits into from

Conversation

addaleax
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@addaleax addaleax added doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. http2 Issues or PRs related to the http2 subsystem. labels Jul 17, 2018
doc/api/http2.md Outdated
stream.end('<h1>Hello World</h1>');
});

server.listen(80);
```

Even though HTTP/2 streams and network sockets are not in a 1:1 correspondence,
a network error will destroy each individual streams and must be handled on the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: streams -> stream?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, done!

@Trott
Copy link
Member

Trott commented Jul 18, 2018

@nodejs/http2

doc/api/http2.md Outdated
@@ -271,12 +271,17 @@ server.on('stream', (stream, headers) => {
'content-type': 'text/html',
':status': 200
});
stream.on('error', (error) => console.log(error));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

console.log -> console.error?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe just:

stream.on('error', console.error);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Switched to the first suggestion – I’d prefer to give a user-defined function here, since that is a bit closer to what one would actually do in real-world code

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 18, 2018
@addaleax
Copy link
Member Author

addaleax commented Jul 18, 2018

@@ -271,12 +271,17 @@ server.on('stream', (stream, headers) => {
'content-type': 'text/html',
':status': 200
});
stream.on('error', (error) => console.error(error));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: how about passing console.error directly? It's already bound. If this is the prevalent usage in the docs, ignore this comment.

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Jul 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@addaleax
Copy link
Member Author

Landed in daa15b5, 0e4cbde

@addaleax addaleax closed this Jul 23, 2018
@addaleax addaleax deleted the http2-aborted-connection branch July 23, 2018 15:05
addaleax added a commit that referenced this pull request Jul 23, 2018
Refs: #21836
Refs: #21561

PR-URL: #21861
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
addaleax added a commit that referenced this pull request Jul 23, 2018
Fixes: #21836

PR-URL: #21861
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
targos pushed a commit that referenced this pull request Jul 24, 2018
Refs: #21836
Refs: #21561

PR-URL: #21861
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
targos pushed a commit that referenced this pull request Jul 24, 2018
Fixes: #21836

PR-URL: #21861
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
@targos targos mentioned this pull request Jul 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants