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

http: added aborted property to request #20094

Closed
wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Apr 17, 2018

Added an aborted property to the request object. Currently, it is not possible to query whether a close event was emitted due to client side socket close without also listening to aborted.

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

@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Apr 17, 2018
@lpinca
Copy link
Member

lpinca commented Apr 17, 2018

Two notes:

  1. Please add a test.
  2. Do we really need this? This is to avoid adding a listener for the 'aborted' event right? What's wrong with it?

@lpinca lpinca added http Issues or PRs related to the http subsystem. http2 Issues or PRs related to the http2 subsystem. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Apr 17, 2018
@ronag
Copy link
Member Author

ronag commented Apr 17, 2018

Do we really need this? This is to avoid adding a listener for the 'aborted' event right? What's wrong with it?

Yes, this basically makes aborted superfluous for http/1 (not for http/2 compat). It's a nice to have.

@mscdex
Copy link
Contributor

mscdex commented Apr 17, 2018

I'm not sure it's worth the overhead of the Date.now() call. Why is it not simply always a boolean, like it's initialized to?

@ronag
Copy link
Member Author

ronag commented Apr 17, 2018

@mscdex: Just following how http.ClientRequest does it.

@ronag
Copy link
Member Author

ronag commented Apr 17, 2018

doc/api/http.md Outdated
@@ -1288,6 +1288,14 @@ the server, then sockets are destroyed when they time out. If a handler is
assigned to the request, the response, or the server's `'timeout'` events,
timed out sockets must be handled explicitly.

### response.aborted
<!-- YAML
added: v10.0.0
Copy link
Member

Choose a reason for hiding this comment

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

This should be added: REPLACEME. The actual version will then be substituted by the releaser when the change first lands in a release.

@ronag ronag force-pushed the http-aborted-req branch 2 times, most recently from 50fe188 to 4c3fe2d Compare April 17, 2018 15:18
@ronag
Copy link
Member Author

ronag commented Apr 17, 2018

Related: 8ad8157

@ronag ronag force-pushed the http-aborted-req branch 3 times, most recently from 8369a98 to 32ee7d5 Compare April 17, 2018 15:41
@Trott
Copy link
Member

Trott commented Apr 17, 2018

@nodejs/http

@mscdex
Copy link
Contributor

mscdex commented Apr 22, 2018

Just following how http.ClientRequest does it.

IMHO that could/should be changed to a boolean as well.

@BridgeAR
Copy link
Member

@ronag is there anything I can help, so it is easier for you to add a test as well? Without a test this PR can not land. Since this has come up in a couple PRs you opened, it would be great to know if there is anything preventing you from writing tests / if there is anything that would help you further.

@ronag
Copy link
Member Author

ronag commented Apr 23, 2018

@BridgeAR It takes several minutes to run the tests using the command ./configure && make -j4 test mentioned in the PR guide. Unless I can run a single test in seconds I just don't have the time and I have no idea how to do that.

@richardlau
Copy link
Member

Unless I can run a single test in seconds I just don't have the time and I have no idea how to do that.

https://github.com/nodejs/node/blob/2060787d386099ceadbb6de2d81dc2f34c90503d/doc/guides/contributing/pull-requests.md#step-6-test

@ronag
Copy link
Member Author

ronag commented Apr 23, 2018

@richardlau: yep, following those instructions, it takes 10 minutes to run.

I can do:

out/Release/node test/parallel/my-test.js

But I still need to rebuild the entire thing as soon as I change anything in lib. Which again takes minutes.

I would like to be able to do something like:

nodemon test/parallel/my-test.js

Which will auto run whenever I change anything in lib or test.

@ronag ronag force-pushed the http-aborted-req branch 3 times, most recently from 314d6fd to 9ffe99b Compare April 23, 2018 10:37
@BridgeAR
Copy link
Member

@ronag

But I still need to rebuild the entire thing as soon as I change anything in lib. Which again takes minutes.

You can run a single test or multiple ones with python tools/test.py -J --mode=release parallel/test-stream2-*. The star indicates a wildcard.

When changing a test you do not have to rebuild the executable. When changing any src or lib files it is necessary to rebuild as with any other regular program.

Doing a minor change after having a up to date build should not take long though. Only the touched files are going to be rebuild and that does not take long (on my machine a couple seconds).
So it is mainly about rebuilding once when starting your PR.

Unless I can run a single test in seconds I just don't have the time

I hope my hints help you already to reduce that overhead. We all have limited time and most collaborators also work on Node.js in their spare time. So we all must go through the same procedure.

@ronag
Copy link
Member Author

ronag commented Apr 23, 2018

@BridgeAR ah, I shouldn't do ./configure && make but just make (Sorry, I'm from the Visual Studio world). Maybe update the guidelines with something like:

Workflow:

make -j8
python tools/test.py -J --mode=release parallel/test-stream2-*

@ronag
Copy link
Member Author

ronag commented Apr 23, 2018

@mscdex @lpinca @BridgeAR with tests.

MylesBorins added a commit that referenced this pull request May 8, 2018
Notable Changes:

* console:
  - make console.table() use colored inspect (TSUYUSATO Kitsune)
    #20510
* fs:
  - move fs/promises to fs.promises (cjihrig)
    #20504
* http:
  - added aborted property to request (Robert Nagy)
    #20094
* n-api:
  - initialize a module via a special symbol (Gabriel Schulhof)
    #20161
* src:
  - add public API to expose the main V8 Platform (Allen Yonghuang Wang)
    #20447

PR-URL: #20606
MylesBorins added a commit that referenced this pull request May 9, 2018
Notable Changes:

* console:
  - make console.table() use colored inspect (TSUYUSATO Kitsune)
    #20510
* fs:
  - move fs/promises to fs.promises (cjihrig)
    #20504
* http:
  - added aborted property to request (Robert Nagy)
    #20094
* n-api:
  - initialize a module via a special symbol (Gabriel Schulhof)
    #20161
* src:
  - add public API to expose the main V8 Platform (Allen Yonghuang Wang)
    #20447

PR-URL: #20606
MylesBorins added a commit that referenced this pull request May 9, 2018
Notable Changes:

* console:
  - make console.table() use colored inspect (TSUYUSATO Kitsune)
    #20510
* fs:
  - move fs/promises to fs.promises (cjihrig)
    #20504
* http:
  - added aborted property to request (Robert Nagy)
    #20094
* n-api:
  - initialize a module via a special symbol (Gabriel Schulhof)
    #20161
* src:
  - add public API to expose the main V8 Platform (Allen Yonghuang Wang)
    #20447

PR-URL: #20606
kjin pushed a commit to kjin/node that referenced this pull request Aug 23, 2018
PR-URL: nodejs#20094
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
kjin pushed a commit to kjin/node that referenced this pull request Sep 11, 2018
PR-URL: nodejs#20094
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
kjin pushed a commit to kjin/node that referenced this pull request Sep 19, 2018
PR-URL: nodejs#20094
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
kjin pushed a commit to kjin/node that referenced this pull request Oct 16, 2018
PR-URL: nodejs#20094
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
Backport-PR-URL: #22850
PR-URL: #20094
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Oct 30, 2018
BethGriggs added a commit that referenced this pull request Nov 20, 2018
Notable changes:

* **assert**:
  - backport some assert commits (Ruben Bridgewater) [#23223](#23223)
* **deps**:
  - V8: cherry-pick 64-bit hash seed commits (Yang Guo) [#23274](#23274)
* **http**:
  - added aborted property to request (Robert Nagy) [#20094](#20094)
* **http2**:
  - backport http2 changes from 10.x (Kelvin Jin) [#22850](#22850)

PR-URL: #23974
MylesBorins pushed a commit that referenced this pull request Nov 20, 2018
Notable changes:

* **assert**:
  - backport some assert commits (Ruben Bridgewater)
    [#23223](#23223)
* **deps**:
  - upgrade to libuv 1.23.2 (cjihrig)
    [#23336](#23336)
  - V8: cherry-pick 64-bit hash seed commits (Yang Guo)
    [#23274](#23274)
* **http**:
  - added aborted property to request (Robert Nagy)
    [#20094](#20094)
* **http2**:
  - graduate from experimental (James M Snell)
    [#22466](#22466)

PR-URL: #23974
MylesBorins pushed a commit that referenced this pull request Nov 20, 2018
Notable changes:

* **assert**:
  - backport some assert commits (Ruben Bridgewater)
    [#23223](#23223)
* **deps**:
  - upgrade to libuv 1.23.2 (cjihrig)
    [#23336](#23336)
  - V8: cherry-pick 64-bit hash seed commits (Yang Guo)
    [#23274](#23274)
* **http**:
  - added aborted property to request (Robert Nagy)
    [#20094](#20094)
* **http2**:
  - graduate from experimental (James M Snell)
    [#22466](#22466)

PR-URL: #23974
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Notable changes:

* **assert**:
  - backport some assert commits (Ruben Bridgewater)
    [nodejs#23223](nodejs#23223)
* **deps**:
  - upgrade to libuv 1.23.2 (cjihrig)
    [nodejs#23336](nodejs#23336)
  - V8: cherry-pick 64-bit hash seed commits (Yang Guo)
    [nodejs#23274](nodejs#23274)
* **http**:
  - added aborted property to request (Robert Nagy)
    [nodejs#20094](nodejs#20094)
* **http2**:
  - graduate from experimental (James M Snell)
    [nodejs#22466](nodejs#22466)

PR-URL: nodejs#23974
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. http Issues or PRs related to the http subsystem. http2 Issues or PRs related to the http2 subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants