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

benchmark: fix async-resource benchmark #33642

Closed

Conversation

addaleax
Copy link
Member

In the benchmark, because it performs asynchronous operations before
writing its HTTP replies, the underlying socket can be closed by the
peer before the response is written. Since 28e6626, that means
that attempting to .end() the HTTP response results in an uncaught
exception, breaking the benchmark.

Fix that by checking whether the response object has been destroyed
or not before attempting to call .end().

#33591

(As noted in the issue, I’m not really happy with this solution.)

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

In the benchmark, because it performs asynchronous operations before
writing its HTTP replies, the underlying socket can be closed by the
peer before the response is written. Since 28e6626, that means
that attempting to `.end()` the HTTP response results in an uncaught
exception, breaking the benchmark.

Fix that by checking whether the response object has been destroyed
or not before attempting to call `.end()`.

nodejs#33591
@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. benchmark Issues and PRs related to the benchmark subsystem. labels May 29, 2020
@BridgeAR
Copy link
Member

BridgeAR commented May 30, 2020

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 30, 2020
@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
@addaleax
Copy link
Member Author

addaleax commented Jun 6, 2020

Landed in f5ed5fe

@addaleax addaleax closed this Jun 6, 2020
addaleax added a commit that referenced this pull request Jun 6, 2020
In the benchmark, because it performs asynchronous operations before
writing its HTTP replies, the underlying socket can be closed by the
peer before the response is written. Since 28e6626, that means
that attempting to `.end()` the HTTP response results in an uncaught
exception, breaking the benchmark.

Fix that by checking whether the response object has been destroyed
or not before attempting to call `.end()`.

#33591

PR-URL: #33642
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
@addaleax addaleax deleted the fix-async-resource-benchmark branch June 6, 2020 15:37
codebytere pushed a commit that referenced this pull request Jun 18, 2020
In the benchmark, because it performs asynchronous operations before
writing its HTTP replies, the underlying socket can be closed by the
peer before the response is written. Since 28e6626, that means
that attempting to `.end()` the HTTP response results in an uncaught
exception, breaking the benchmark.

Fix that by checking whether the response object has been destroyed
or not before attempting to call `.end()`.

#33591

PR-URL: #33642
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
@codebytere codebytere mentioned this pull request Jun 28, 2020
codebytere pushed a commit that referenced this pull request Jun 30, 2020
In the benchmark, because it performs asynchronous operations before
writing its HTTP replies, the underlying socket can be closed by the
peer before the response is written. Since 28e6626, that means
that attempting to `.end()` the HTTP response results in an uncaught
exception, breaking the benchmark.

Fix that by checking whether the response object has been destroyed
or not before attempting to call `.end()`.

#33591

PR-URL: #33642
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
codebytere pushed a commit that referenced this pull request Jul 9, 2020
In the benchmark, because it performs asynchronous operations before
writing its HTTP replies, the underlying socket can be closed by the
peer before the response is written. Since 28e6626, that means
that attempting to `.end()` the HTTP response results in an uncaught
exception, breaking the benchmark.

Fix that by checking whether the response object has been destroyed
or not before attempting to call `.end()`.

#33591

PR-URL: #33642
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
@codebytere codebytere mentioned this pull request Jul 13, 2020
codebytere pushed a commit that referenced this pull request Jul 14, 2020
In the benchmark, because it performs asynchronous operations before
writing its HTTP replies, the underlying socket can be closed by the
peer before the response is written. Since 28e6626, that means
that attempting to `.end()` the HTTP response results in an uncaught
exception, breaking the benchmark.

Fix that by checking whether the response object has been destroyed
or not before attempting to call `.end()`.

#33591

PR-URL: #33642
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. benchmark Issues and PRs related to the benchmark subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants