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

[v13.x backport] stream: don't emit 'finish' after 'error' #32372

Closed
wants to merge 32 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Mar 19, 2020

PR-URL: #32275

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

rvagg and others added 30 commits March 13, 2020 16:36
Includes hardened-runtime patch from gdams from
nodejs#29216 (comment)

PR-URL: nodejs#31459
Refs: nodejs#29216
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ash Cripps <ashley.cripps@ibm.com>
Signed-off-by: Rod Vagg <rod@vagg.org>
PR-URL: nodejs#31459
Refs: nodejs#29216
Refs: sindresorhus/macos-terminal-size#3
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ash Cripps <ashley.cripps@ibm.com>
Signed-off-by: Rod Vagg <rod@vagg.org>
PR-URL: nodejs#31459
Refs: nodejs#29216
Reviewed-By: Christian Clauss <cclauss@me.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ash Cripps <ashley.cripps@ibm.com>
Signed-off-by: Rod Vagg <rod@vagg.org>
I sent an email to inactive collaborators (in the previous 6 months at
the time, no commits, no PRs reviewed, and no commits landed) asking if
it was time to move to emeritus.

Some replied affirmatively and I have moved those people to emeritus.

Others replied indicating a desire to remain collaborators, and I left
them as collaborators.

The remaining folks are being moved to emeritus in this change.

Signed-off-by: Rich Trott <rtrott@gmail.com>

PR-URL: nodejs#32151
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@hiroppy.me>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Signed-off-by: Michael Dawson <michael_dawson@ca.ibm.com>

PR-URL: nodejs#32213
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Matheus Marchini <mat@mmarchini.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
PR-URL: nodejs#31977
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: nodejs#32037
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Fixes broken unit test for the NODE_EXTRA_CA_CERTS environment
variable. Unit test was exiting without evaluating any assertions
or running any tests.

Fixes: nodejs#32072

PR-URL: nodejs#32073
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: nodejs#32087
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Make sure longer strings are written up to the buffer end

Refs: nodejs#32119

PR-URL: nodejs#32123
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: nodejs#32125
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Different parts of the debug build were using differently sized
std::vectors due to `_GLIBCXX_DEBUG` sometimes being defined and
sometimes not. That ended about as well as you would expect.

Remove the flag.

Fixes: nodejs#30056

PR-URL: nodejs#30147
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Define `util.promisify.custom`
as `Symbol.for("nodejs.util.inspect.custom")`, rather than
as `Symbol("util.inspect.custom")`.

This allows custom `promisify` wrappers to easily/safely be defined
in non‑Node.js environments.

Fixes: nodejs#31647

PR-URL: nodejs#31672
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Document Windows specific fs.watch caveats.

Fixes: nodejs#31702

PR-URL: nodejs#32176
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Masashi Hirano <shisama07@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
This adds preview output for input that may not be wrapped.

PR-URL: nodejs#32154
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Use an object to indicate which part belongs to the input and which
to the output.
On top of that this also simplifies the expected output by
automatically inserting the default repl line for previews and by
automatically checking for the correct output line length.

PR-URL: nodejs#32154
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Co-Authored-By: Geoffrey Booth <GeoffreyBooth@users.noreply.github.com>
PR-URL: nodejs#32098
Reviewed-By: Geoffrey Booth <webmaster@geoffreybooth.com>
The explicit goal is to let users use `import.meta.url` to
re-load thecurrent module inside a Worker instance.

Fixes: nodejs#30780
PR-URL: nodejs#31664
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
The test wanted to cut huge string into 1KB strings,
for which a new line character was inserted at appropriate
places. The value is different in Windows (10, 13).
Make it portable, by making use of os.EOL semantics

Refs: nodejs#25988

PR-URL: nodejs#32104
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
This change is to prevent potential bugs - e.g., someone might
automatically use the variable `k` instead of `key`, that is used in
vicinity of this loop.
Also changed postincrement to preincrement in iteration steps. It is
probably done by the optimizer anyway, but otherwise it will save an
opcode each iteration. And it is a good practice.

PR-URL: nodejs#32012
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Add a new scenario of multiple clients sharing a single data
callback function managing their response data through
AsyncLocalStorage APIs

Refs: nodejs#32063
Refs: nodejs#32060
Refs: nodejs#32062 (comment)

Co-authored-by: Gireesh Punathil <gpunathi@in.ibm.com>

PR-URL: nodejs#32082
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Original commit message:

    Fix scanner-level error reporting for hashbang

    When the file begins with a hashbang, the scanner is in a failed state
    when SkipHashbang() is called. This is usually not an issue but when
    the parser encounters an ILLEGAL token, it will reset the SyntaxError
    location because of it.

    Bug: v8:10110
    Change-Id: I1c7344bf5ad20079cff80130c991f3bff4d7e9a8
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1995312
    Reviewed-by: Toon Verwaest <verwaest@chromium.org>
    Commit-Queue: Toon Verwaest <verwaest@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#66038}

Refs: v8/v8@f925780
Fixes: nodejs#31284
Signed-off-by: Matheus Marchini <mmarchini@netflix.com>

PR-URL: nodejs#32180
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: nodejs#32203
Fixes: nodejs#32199
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
There is no reason for this to be in C++. Using JavaScript means that
the code is more accessible to more developers, which is important
for any Node.js feature. This also simplifies the code significantly
in some areas. On the technical side, this potentially also enables
making some of the file system operations that are involved
asynchronous.

PR-URL: nodejs#32201
Reviewed-By: Bradley Farias <bradley.meck@gmail.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
This commit adds a test to verify that exceptions thrown from a
WASI application are properly caught and rethrown. This also
gets the code coverage in lib/wasi.js back to 100%.

PR-URL: nodejs#32157
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Notable changes:

- The UV_UDP_MMSG_CHUNK UDP flag has been added.
- Support has been dropped for FreeBSD < 10.
- The FreeBSD and Linux system call logic has been simplified to assume the
  presence of features covered by libuv's minimum system requirements.
- Listening on IPC pipes is no longer allowed.
- Fix iOS and Android builds.

PR-URL: nodejs#32204
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Update acorn to 7.1.1 in the dev dependencies for the markdown linter.

Refs: https://app.snyk.io/vuln/SNYK-JS-ACORN-559469
Signed-off-by: Rich Trott <rtrott@gmail.com>

PR-URL: nodejs#32259
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
The current test relies on undeterministic behavior from V8 GC, and on
newer versions of V8 this test ocasionally fails because that behavior
changed.  Prevent that from happening using --predictable-gc-schedule.
If this test fails in the future, it should fail consistently instead of
ocasionally, which should help debug.

Ref: nodejs/node-v8#144 (comment)
Signed-off-by: Matheus Marchini <mmarchini@netflix.com>

PR-URL: nodejs#32239
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
../src/node_file.cc:386:7: warning: ignoring return value of
function declared with 'warn_unused_result'
attribute [-Wunused-result]

PR-URL: nodejs#32241
Refs: nodejs#31972
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
`test-inspector-multisession-ws` and `test-inspector-break-when-eval`
will be affected by an upstream bug when we upgrade V8 to 8.1. The bug
is caused when the Inspector sets a pause at the start of a function
compiled with `CompileFunctionInContext`, but that function hasn't been
executed yet.

On both tests, this issue is triggered by pausing while in C++ executing
LookupAndCompile, which is called by requiring internal modules while
running `console.log`. To eliminate this issue in both tests, we add an
extra `console.log` to ensure we only pause we required all internal
modules we need. On `test-inspector-break-when-eval`, we also need to
start the child process with `--inspect-brk` instead of `--inspect` to
ensure the test is predictable (this test would occasianlly fail on
slower machines, when console.log doesn't run fast enough to finish
after emitting `Runtime.consoleAPICalled` and before the parent process
sending `Runtime.evaluate` message.

Ref: https://bugs.chromium.org/p/v8/issues/detail?id=10287

PR-URL: nodejs#32234
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=10287
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Use `dl_iterate_phdr(3)` to find the mapping containing
`__node_text_start` instead of parsing `/proc/self/maps`.

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: nodejs#32244
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: David Carlier <devnexen@gmail.com>
@nodejs-github-bot nodejs-github-bot added stream Issues and PRs related to the stream subsystem. v13.x zlib Issues and PRs related to the zlib subsystem. labels Mar 19, 2020
PR-URL: nodejs#32275
Refs: nodejs#28710
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Backport-PR-URL: nodejs#32372
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

MylesBorins pushed a commit that referenced this pull request Mar 24, 2020
Backport-PR-URL: #32372
PR-URL: #32275
Refs: #28710
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Backport-PR-URL: #32372
@MylesBorins
Copy link
Contributor

backport landed in 531d495

MylesBorins pushed a commit that referenced this pull request Mar 24, 2020
Backport-PR-URL: #32372
PR-URL: #32275
Refs: #28710
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Backport-PR-URL: #32372
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.