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 http "aborted" events #7270

Closed

Conversation

kemitchell
Copy link
Contributor

@kemitchell kemitchell commented Jun 10, 2016

Checklist
  • make -j4 test (UNIX) or vcbuild test nosign (Windows) passes (This PR just adds doc)
  • a test and/or benchmark is included (This PR just adds doc)
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

doc (for HTTP)

Description of change

This pull request follows on #6925 (in which @Fishrock123 and @dougwilson made key appearances) to document the "aborted" (as opposed to "abort") events emitted on HTTP requests and responses on socket closure by the remote system.

A few links from the issue that seem most relevant:

The last tests the event on req from the server point of view. I actually don't see a test checking the event from the client API. This test sets a handler on a res from an http.get call, but as far as I can tell there's no test verifying its functionality. I can try and cook one up if needed.

_Be Warned! This is my first PR to Node.js. I reread CONTRIBUTING, but probably screwed something up anyway. Many thanks to team for patience and understanding!_

This PR coming to you live from doc sprint at NodeConf Adventure.

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jun 10, 2016
@mscdex mscdex added the http Issues or PRs related to the http subsystem. label Jun 11, 2016
@addaleax
Copy link
Member

I think you can squash the commits together and add an Fixes: https://github.com/nodejs/node/issues/6925 line (of course, that can also be done upon landing).

This looks good to me, but my knowledge of the HTTP implementation in core is not too extensive, so /cc @nodejs/http

@jasnell
Copy link
Member

jasnell commented Jun 20, 2016

LGTM, the commits can definitely be squashed but that can be done by whomever lands.

@kemitchell
Copy link
Contributor Author

Separate PR for a test checking aborted events on client response objects? Or am I still missing something?

@addaleax
Copy link
Member

Landed in 86fdbe0

Many thanks, and I think it’s safe to say that a separate PR for testing this event on client responses would be appreciated! :)

@addaleax addaleax closed this Jun 22, 2016
addaleax pushed a commit that referenced this pull request Jun 22, 2016
Fixes: #6925
PR-URL: #7270
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@kemitchell kemitchell deleted the document-http-aborted-event branch June 22, 2016 23:32
Fishrock123 pushed a commit that referenced this pull request Jun 27, 2016
Fixes: #6925
PR-URL: #7270
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@Fishrock123 Fishrock123 mentioned this pull request Jun 27, 2016
Fishrock123 pushed a commit that referenced this pull request Jul 5, 2016
Fixes: #6925
PR-URL: #7270
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@Fishrock123 Fishrock123 mentioned this pull request Jul 5, 2016
MylesBorins pushed a commit that referenced this pull request Jul 11, 2016
Fixes: #6925
PR-URL: #7270
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Fixes: #6925
PR-URL: #7270
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Fixes: #6925
PR-URL: #7270
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Fixes: #6925
PR-URL: #7270
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Fixes: #6925
PR-URL: #7270
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Fixes: #6925
PR-URL: #7270
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants