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

v8.3.0 proposal #14594

Merged
merged 157 commits into from
Aug 10, 2017
Merged

v8.3.0 proposal #14594

merged 157 commits into from
Aug 10, 2017

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Aug 2, 2017

For real this time.

2017-08-??, Version 8.3.0 (Current), @addaleax

Notable changes

V8 6.0

The V8 engine has been upgraded to version 6.0, which has a significantly
changed performance profile.
#14574

More detailed information on performance differences can be found at
https://medium.com/the-node-js-collection/get-ready-a-new-v8-is-coming-node-js-performance-is-changing-46a63d6da4de

Other notable changes

  • DNS

    • Independent DNS resolver instances are supported now, with support for
      cancelling the corresponding requests.
      #14518
  • REPL

    • Autocompletion support for require() has been improved.
      #14409
  • Utilities

    • The WHATWG Encoding Standard (TextDecoder and TextEncoder) has
      been implemented.
      #13644
  • Added new collaborators

Commits

BridgeAR and others added 30 commits July 24, 2017 10:58
Backport-PR-URL: #14392
Backport-Reviewed-By: James M Snell <[email protected]>

PR-URL: #13733
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
The way it is currently written, test-http-full-response will fail if
there is a problem with spawning that doesn't include `ab` or `api` in
`stderr`, but it will fail with a misleading mismatched-calls
`common.mustCall()` error.

Alter the error handling so that it rethrows the actual error, providing
better information.

PR-URL: #14252
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Adds a missing `'` in code example.

PR-URL: #14353
Fixes: #14352
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Use template literals instead of string concatenation in
test/parallel/test-http-extra-response.js

PR-URL: #14296
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: David Cai <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Add a note to the stream docs specifying that at most a single
call to _transform can happen, and the provided callback()
should be used to process another chunk.

Fixes: #3208
PR-URL: #14321
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Previous behavior was to assume an error is a proper error in the
repl module. A check was added to not terminate the process on thrown
repl errors that are `null` or `undefined`.

PR-URL: #14306
Fixes: #12373
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]
PR-URL: #14372
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #14287
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
fs.rmdir() on the file (not directory) results in different errors on
Windows to everything else

Fixes: #8797
PR-URL: #14323
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
PR-URL: #14342
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Allow passing the prefix in via the PKGDIR env var. This will allow us
to use this same script to codesign the binary tarball.

PR-URL: #14179
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Previously, we were signing the binary that was released in the .pkg,
but not the binary released in the tarball.

PR-URL: #14179
Fixes: #11936
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: #14272
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
PR-URL: #14280
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
test-fs-largefile was disabled. It was fixed in bbf74fb but left in
disabled because it generates a 5Gb file. However, gibfahn had the
sensible suggestion of moving it to the pummel directory. Which is what
this change does.

In pummel, lint rules are applied, so this does necessitate changing a
pair of `var` declarations to `const`.

PR-URL: #14338
Refs: bbf74fb
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: #14388
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Clarifies the default shell in Windows is process.env.ComSpec
and that cmd.exe is only used as a fallback. Functions whose
descriptions are affected include:
child_process.spawn, child_process.exec,
child_process.spawnsSync, and child_process.execSync.

PR-URL: #14203
Fixes: #14156
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Added comments to whatwg-url tests that they should not be changed until
modifications are merged upstream as per "Web Platform Tests" guidelines

PR-URL: #14355
Fixes: #12793
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: #14364
Fixes: #14362
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
PR-URL: #14319
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Vse Mozhet Byt <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
PR-URL: #14286
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Line 486 and 525 contain for loops where a length property is cached in
a variable (for example, itemLen). This used to be a performance
optimization, but current V8 handles the optimization internally.

These caching variables are removed, and the length property is used
directly instead.

PR-URL: #14275
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
By adding a test case using a path with illegal protocol

PR-URL: #14301
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
This test was disabled in 2011 and is no longer useful without
modifications.

PR-URL: #14334
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Replace 250ms timer with event-based logic to make test robust.

PR-URL: #14361
Fixes: #13597
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: #14375
Refs: #13937
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Move test-http-server-keep-alive-timeout-slow-server and
test-http-server-keep-alive-timeout-slow-client-headers from parallel to
sequential to resolve test flakiness on freebsd10-64.

Fixes: #14033
Refs: #9317
PR-URL: #14377
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Also used in common.gypi to check whether a flag is needed or not
based on llvm version.

PR-URL: #14077
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
addaleax and others added 5 commits August 9, 2017 20:07
Fix situations in which the handle passed along with a message
that has a large payload and can’t be read entirely by a single
`recvmsg()` call isn’t associated with the message to which it
belongs.

PR-URL: #14588
Fixes: #13778
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Unset `mode_` when initializing the zlib stream failed, so that
we don’t try to call the zlib end functions (`deflateEnd()` etc.)
when cleaning up in `ZCtx::Close()`.

Fixes: #14178
Ref: #13098
PR-URL: #14666
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Alexey Orlenko <[email protected]>
Fixes a performance regression in body-parser with V8 6.0.
Removes the use of an auxiliary array, and just query the object
directly.

PR-URL: #14703
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #14692
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
V8 6.0:

  The V8 engine has been upgraded to version 6.0, which has a significantly
  changed performance profile.
  [#14574](#14574)

  More detailed information on performance differences can be found at
  https://medium.com/the-node-js-collection/get-ready-a-new-v8-is-coming-node-js-performance-is-changing-46a63d6da4de

Other notable changes:

* **DNS**
  * Independent DNS resolver instances are supported now, with support for
    cancelling the corresponding requests.
    [#14518](#14518)

* **N-API**
  * Multiple N-API functions for error handling have been changed to support
    assigning error codes.
    [#13988](#13988)

* **REPL**
  * Autocompletion support for `require()` has been improved.
    [#14409](#14409)

* **Utilities**
  * The WHATWG Encoding Standard (`TextDecoder` and `TextEncoder`) has
    been implemented as an experimental feature.
    [#13644](#13644)

* **Added new collaborators**
  * [XadillaX](https://github.com/XadillaX) – Khaidi Chu
  * [gabrielschulhof](https://github.com/gabrielschulhof) – Gabriel Schulhof
@addaleax
Copy link
Member Author

addaleax commented Aug 9, 2017

I’ve updated this PR with all remaining PRs that contain fixes for confirmed bugs/regressions, including #14703 , plus ad901ed because it’s harmless and fits in well as a notable change.

I’m not sure what to do given that CI isn’t up. A local make test passes, though.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 9, 2017

Kicked off the release CI with the new changes.

@MylesBorins
Copy link
Contributor

We should hold off on a release until we can test with Ci + Citgm

@cjihrig
Copy link
Contributor

cjihrig commented Aug 9, 2017

Agreed. But the ARM release job is still running, so I'm glad I kicked it off when I did. Any word on the CI?

Copy link
Contributor

@evanlucas evanlucas left a comment

Choose a reason for hiding this comment

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

LGTM if ci is happy

@addaleax
Copy link
Member Author

CITGM looks good (although AIX failed completely), CI is not exactly green and has a couple flaky tests + infra failures but nothing new or concerning (imo) :shipit: (one ARM machine is still running right now)

@cjihrig
Copy link
Contributor

cjihrig commented Aug 10, 2017

@MylesBorins are you able to sign off on this?

@refack
Copy link
Contributor

refack commented Aug 10, 2017

The node-iconv CitGM showed on windows does not repro locally.

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

CitGM is clean (i.e. no new failures)

@MylesBorins
Copy link
Contributor

Quick follow up on CITGM

Three modules seem legitimately broken right now, a few more seem flaky (not sure if it is our CI systems or the modules themselves.)

It doesn't appear that anything major is broken with this release.

Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LGTM

PR-URL: #14594
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Reviewed-By: Evan Lucas <[email protected]>
@cjihrig cjihrig merged commit 08b662a into v8.x Aug 10, 2017
@cjihrig cjihrig deleted the v8.3.0-proposal branch August 10, 2017 02:02
cjihrig added a commit to nodejs/nodejs.org that referenced this pull request Aug 10, 2017
@vsemozhetbyt
Copy link
Contributor

RE nodejs/nodejs.org#1334: are not "add console.count() and console.clear()" notable changes?

@kyrylkov
Copy link

kyrylkov commented Aug 10, 2017

@addaleax @cjihrig are spread properties for object literals not supported in v8.3.0?

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Aug 10, 2017

@kyrylkov Supported:

> process.versions.node
'8.3.0'
> process.versions.v8
'6.0.286.52'
> const { key1, ...rest } = { key1: 1, key2: 2, key3: 3 };
undefined
> rest
{ key2: 2, key3: 3 }
> const copy = { key1: 1, ...rest };
undefined
> copy
{ key1: 1, key2: 2, key3: 3 }

Refs: https://v8project.blogspot.com/2017/06/v8-release-60.html

@kyrylkov
Copy link

@vsemozhetbyt Yep, sorry. babel-node issue.

> process.versions.node
'8.3.0'
> process.versions.v8
'6.0.286.52'
> const { key1, ...rest } = { key1: 1, key2: 2, key3: 3 };
SyntaxError: repl: Unexpected token (1:14)
> 1 | const { key1, ...rest } = { key1: 1, key2: 2, key3: 3 };
    |               ^

@mhdawson
Copy link
Member

A bit late but I agree changing the N-API version is not necessary quite yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.