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

Doc describes HTTP "abort" event, but not "aborted" #6925

Closed
kemitchell opened this issue May 23, 2016 · 5 comments
Closed

Doc describes HTTP "abort" event, but not "aborted" #6925

kemitchell opened this issue May 23, 2016 · 5 comments
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. http Issues or PRs related to the http subsystem.

Comments

@kemitchell
Copy link
Contributor

kemitchell commented May 23, 2016

  • Version: 6.2.0
  • Platform: Linux 3.16.0-4-amd64 # 1 SMP Debian 3.16.7-ckt25-2 (2016-04-08) x86_64 GNU/Linux
  • Subsystem: HTTP

TL;DR: HTTP doc mentions 'abort' event, but not 'aborted'. The API emits both. Doc fix?

Current doc describes an 'abort' event on HTTP streams here. That doc was added in 2ca22aa, which closed #945. #945 seems to deal with the case where .abort() is called on a client only. It refers to nodejs/node-v0.x-archive#9278. As far as I can tell, current doc never mentions an 'aborted' event.

The HTTP client emits 'abort' from requests here.

The HTTP client also emits 'aborted' from the corresponding response (req.res) here.

The HTTP server emits 'aborted' from requests here.

'aborted' events make it to user space. raw-body, a dep of the popular body-parser, listens for it here.

A few questions:

  1. Is aborted intentionally undocumented? I see it mentioned in the CHANGELOG archives a couple of times from the 0.4.x days, here and originally (on Agent) here. Would the team welcome a doc patch mentioning it?
  2. Are abort and aborted intentionally distinct events?

_Be Warned! I believe this is my first issue on Node. I've read CONTRIBUTING and friends and done my best._

Thanks to the team!

@Fishrock123 Fishrock123 added the http Issues or PRs related to the http subsystem. label May 23, 2016
@Fishrock123
Copy link
Contributor

Fishrock123 commented May 23, 2016

@nodejs/http

Edit: Although I suspect there is little we can do about it at this stage? (Besides docs)

@dougwilson
Copy link
Member

AFAIK, they are distinct events. The abort event is emitted only in the client, to correspond to when a request was manually aborted from client code (calling req.abort()) while the aborted event is an event at the req/res level shared between the server and client code, and indicates that the request or response was aborted (from the other end only, I think).

I believe they were named differently because they have different meanings, and the aborted event already existed.

The current documentation for abort looks correct to me. I think the best course of action would be to also document the aborted event and what it means (the other end aborted the request/response).

@mscdex mscdex added the doc Issues and PRs related to the documentations. label May 23, 2016
@kemitchell
Copy link
Contributor Author

The test for 'aborted'

@kemitchell
Copy link
Contributor Author

@dougwilson: For the record, my cold read of #945 and the tests matches your summary of 'abort' v. 'aborted'. I'll be happy to PR doc for 'aborted' if needed.

@Fishrock123 Fishrock123 added the good first issue Issues that are suitable for first-time contributors. label May 25, 2016
@Fishrock123
Copy link
Contributor

@kemitchell I think we'd accept a PR to document it. :)

Fishrock123 pushed a commit that referenced this issue Jun 27, 2016
Fixes: #6925
PR-URL: #7270
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Fishrock123 pushed a commit that referenced this issue Jul 5, 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 issue 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 issue 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 issue 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 issue 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 issue 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 issue 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. good first issue Issues that are suitable for first-time contributors. http Issues or PRs related to the http subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants