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.1.3 proposal #13861

Merged
merged 62 commits into from
Jun 29, 2017
Merged

v8.1.3 proposal #13861

merged 62 commits into from
Jun 29, 2017

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jun 21, 2017

After talking with @mcollina for a bit, we agreed that it would be best to do a patch release soon rather than waiting for 8.2.0, so this is a light version of that proposal. We’d like to include #13850 once that lands, but that should be about it.

I’ve marked 8.2.0 as blocked for now, but if any of the release people have the time to create RC builds for 8.2.0, I think that would still be a good way to get feedback on TF+I in Node 8.

/cc @nodejs/release @nodejs/ctc


2017-06-??, Version 8.1.3 (Current), @addaleax

Notable changes

  • Stream
    Two regressions with the stream module have been fixed:
    • The finish event will now always be emitted after the error event
      if one is emitted:
      [0a9e96e86c]
      #13850
    • In object mode, readable streams can now use undefined again.
      [5840138e70]
      #13760

Commits

seishun and others added 30 commits June 21, 2017 22:42
PR-URL: #11607
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
test-child-process-stdio-big-write-end was failing on ubuntu1604-arm64
because the while loop that was supposed to fill up the buffer ended up
being an infinite loop.

This increases the size of the writes in the loop by 1K until the buffer
fills up.

PR-URL: #13626
Fixes: #13603
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Confirm that callback is not being invoked in
test-http-eof-on-connect.js.

PR-URL: #13587
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
The earlier version `napi_callback` returns `void` but now is
`napi_value`. The document of this section hasn't been modified.

PR-URL: #13570
Fixes: #12248
Fixes: #13562
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #13620
Fixes: #13616
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Minor correction in the comment regarding ssl_set_pkey.

PR-URL: #13653
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Add common.mustCall() check to test-child-process-stdio-big-write-end.

PR-URL: #13605
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Use common.mustNotCall() in test/sequential/test-fs-watch.js in
situations where the call to watch() is expected to throw.

PR-URL: #13595
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #13561
Ref: #13452
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Add a note to clarify that any platform that is EoL will not be
supported by Node.js.

PR-URL: #12672
Fixes: nodejs/build#688
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Updated onboarding-extras.md to /cc @nodejs/v8-inspector for inspector
issues and reorganized /cc list in alphabetical order.

PR-URL: #13632
Fixes: #13621
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Also, add tests to ensure they will always return this, and to confirm
they return this when these doc changes are back-ported to earlier
release lines.

PR-URL: #13531
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
* block scope `paused`
* change name of block-scoped `file3` etc. to `file`
* alphabetize modules
* confirm contents provided in `data` callback
* confirm `data` callbacks will not fire on tests for errors

PR-URL: #13618
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
> Generally it will correspond the name of the resource's constructor.
should read "Generally, it will correspond to the name..."

PR-URL: #13666
Fixes: #13663
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
In non-buffer tests, change usage of the Buffer constructor to one of
the recommended alternatives.

PR-URL: #13649
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Brian White <mscdex@mscdex.net>
* use common.mustNotCall() to confirm callback is not invoked
* blank line after common module per test writing guide

PR-URL: #13661
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
* Adding --inspect-port with debug port, instead of parsing `execArgv`

* Export CLI debug options to `process.binding('config').debugOptions`
  (currently used only in tests)

PR-URL: #13619
Refs: #9659
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
oath.md: make order of properties consistent
tls.md: remove spaces in getPeerCertificate signature
tls.md: add deprecation notice to server.connections
http.md: fix signature of request.end
crypto.md: change crypto parameters to camelCase
vm.md: add missing apostrophe
vm.md: fix signature of vm.runInNewContext
zlib.md: improve description of zlib.createXYZ

PR-URL: #13491
Ref: #9538
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
- Add doc for napi_create_string_latin1().
- Fix signatures where c string was specified instead of napi_value.
- Fix return type of napi_callback.
- Update to specify that napi_escape_handle() can only be called once
  for a given scope.

PR-URL: #13650
Fixes: #13555
Fixes: #13556
Fixes: #13562
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jason Ginchereau <jasongin@microsoft.com>
Reviewed-By: Ingvar Stepanyan <me@rreverser.com>
There are a number of void casts of clear_error_on_return which is a
usage of the RAII idiom. The ClearErrorOnReturn struct only has a
destructor and no constructor which I believe was an issue in GCC
prior to version 4.8.0, which lead to a unused variable warning.

I'm wondering if these cast could be removed since GCC 4.8.5 or newer
is required now. An alternative solution would be to add an empty
constructor which should work allowing the compiler to detect that a
variable is used only for its side-effects.

Not sure if this was the sole reason for having these casts but wanted
to bring it up just in case.

Refs: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=10416
PR-URL: #13669
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
* re-implemented test to parse args instead of post binding (exit 12)
* saved failing case as known issue

PR-URL: #13373
Fixes: #13343
Reviewed-By: James M Snell <jasnell@gmail.com>
* Add common.mustCall().
* Add check for error existence, not only for error details.
* Use test(), not match() in a boolean context.
* Properly reverse meanings of assert messages.

PR-URL: #13680
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: #13685
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: David Cai <davidcai1993@yahoo.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
In util.format, if a percent sign without a known type is encountered,
just print it instead of silently ignoring it and the next character.

PR-URL: #13674
Fixes: #13665
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
`max_i` should also include the characters that were just read by
`base64_decode_group_slow()`.

PR-URL: #13660
Fixes: #13636
Fixes: #13657
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
* alphabetize modules
* replace callback invocation counts with common.mustCall()
* add block-scoping
* remove commented-out code that uses an identifier not in the test
* move exit handlers to relevant blocks to keep tests cohesive
* common.noop -> common.mustNotCall()
* check results from `data` event

PR-URL: #13643
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #13673
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #13658
Ref: #13645 (comment)
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
PR-URL: #13413
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
PR-URL: #13713
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@addaleax
Copy link
Member Author

@rvagg I don’t have much docker experience, so I really don’t have any idea how to get core dumps either. If you think giving me access is okay, I’ll see what I can do, but I can’t promise anything.

@refack Mostly, yes. To be clear, I never expected 8.2.0 to be released quickly, it’s a pretty big change, and I do think we should get out the streams fixes soon-ish.

@rvagg
Copy link
Member

rvagg commented Jun 22, 2017

@addaleax: ssh root@107.170.75.204 and you should get in with one of your keys in github. Run docker exec -ti node-ci-alpine-test /bin/bash when you're in and you'll be inside the container running this stuff, just treat that like a normal linux machine and you'll be fine. Alternatively, run docker run -ti --rm node-ci:alpine-test /bin/bash and you'll be in a copy of the image where you can mess around all you like and when you exit it'll disappear. That may be a good approach if you want to build Node and run it in debug or anything else. Treat it like a normal Linux server, but Alpine so you have to use apk to install stuff that may not be there.

@addaleax
Copy link
Member Author

@rvagg Thanks, I tried it but I couldn’t get it to fail manually. I’m afraid I’m done for today. :/

It’s probably really best to take a look at the errors coming from a normal valgrind cctest run on a local machine. But as mentioned, it’s not a new problem that’s new in this release and we haven’t seen any bug reports about this.

@rvagg
Copy link
Member

rvagg commented Jun 23, 2017

Just chatting with @MylesBorins and we're thinking that it'd be best to defer this until next week because of the crappy nature of Friday releases. There's nothing super urgent here that would override that is there? (the non-Friday thing is normally strict for LTS which generally happens on Tuesday but I don't believe we've ever had a strong convention for Current releases).

@mcollina
Copy link
Member

#13850 is the only half-urgent one. But early next week is ok, too. This should be added to this release proposal. It is already in master.

cjihrig and others added 9 commits June 24, 2017 12:19
This commit aims to improve the documentation examples that send
sockets over IPC channels. Specifically, pauseOnConnect is added
to a server that inspects the socket before sending and a
'message' handler adds a check that the socket still exists.

PR-URL: #13196
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
When _write completes with an Error, 'finish' was emitted before
'error' if the callback was asynchronous. This commit restore the
previous behavior.
The logic is still less then ideal, because we call the write()
callback before emitting error if asynchronous, but after if
synchronous. This commit do not try to change the behavior.
This commit fixes a regression introduced by:
#13195.

Fixes: #13812
PR-URL: #13850
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Calvin Metcalf <calvin.metcalf@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
test-global-console-exists cannot use the common module as explained in
a comment but it was included later anyway. This change removes it.

PR-URL: #13748
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
In preparation for applying the more strict indentation linting
available in ESLint 4.0.0, correct minor indentation issues in
tools/eslint-rules/required-modules.js.

This is the only file with indentation that does not conform to the
stricter checks.

PR-URL: #13758
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
ESLint 4.0.0 provides stricter (and more granular) indentation checking
than previous versions. Apply the stricter indentation rules to the
tools directory.

PR-URL: #13758
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Fix previously-unnoticed typo in `required-modules.js`.

Refs: #13758 (comment)
PR-URL: #13758
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
* Use common.mustCall() to confirm that _write() is called only once.
* Check that _write() is called with the correct argument

PR-URL: #13823
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
* Use `common.mustCall()` to track callback invocations
* Remove console.log() statements unrelated to the test
* Add blank line to conform with test-writing guide

PR-URL: #13802
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
* also update STYLE_GUIDE comment about Em dashes

PR-URL: #13749
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
@addaleax
Copy link
Member Author

addaleax commented Jun 24, 2017

I’ve updated this proposal with the other streams fix and the test/doc changes since the last update.

CI: https://ci.nodejs.org/job/node-test-commit/10768/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/889/

@mcollina
Copy link
Member

Are we releasing 8.1.3 this week?

@addaleax
Copy link
Member Author

Are we releasing 8.1.3 this week?

Unless anybody objects to that, I think that’s the plan. (@rvagg ?)

@refack
Copy link
Contributor

refack commented Jun 28, 2017

This needs to wait for #13969

@addaleax
Copy link
Member Author

@refack Why? The original PR isn’t included so my first thought would be that the revert isn’t needed either.

@refack
Copy link
Contributor

refack commented Jun 28, 2017

@refack Why? The original PR isn’t included so my first thought would be that the revert isn’t needed either.

Ok. Makes sense. Hit the "panic" button too soon.

@refack
Copy link
Contributor

refack commented Jun 28, 2017

so my first thought would be that the revert isn’t needed either.

Had a third thought; Nightlies... They will break.
But that should be discussed in the PR.

Notable changes

* **Stream**
  Two regressions with the `stream` module have been fixed:
  * The `finish` event will now always be emitted after the `error` event
    if one is emitted:
    [[`0a9e96e86c`](0a9e96e86c)]
    [#13850](#13850)
  * In object mode, readable streams can now use `undefined` again.
    [[`5840138e70`](5840138e70)]
    [#13760](#13760)
@rvagg
Copy link
Member

rvagg commented Jun 29, 2017

Test run @ https://ci.nodejs.org/job/node-test-commit/10807/ looks great excl the expected Alpine segfault and UCS2 error on the Pi's.
I've taken ownership of the HEAD commit, I feel bad overwriting your name @addaleax but I think for signing and verification the author should match the signer (I may be wrong since the tag is distinct from the commit but I don't want really want to risk it).
Release builds in progress.

@rvagg rvagg merged commit 01461af into v8.x Jun 29, 2017
@rvagg rvagg deleted the v8.1.3-proposal branch June 29, 2017 07:03
@rvagg
Copy link
Member

rvagg commented Jun 29, 2017

thanks for the hard work @addaleax, release is done

@mcollina
Copy link
Member

Thanks folks!

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.