Skip to content

Conversation

@addaleax
Copy link
Member

@addaleaxaddaleax 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

seishunand others added 30 commits June 21, 2017 22:42
PR-URL: #11607 Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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: #13626Fixes: #13603 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Confirm that callback is not being invoked in test-http-eof-on-connect.js. PR-URL: #13587 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
The earlier version `napi_callback` returns `void` but now is `napi_value`. The document of this section hasn't been modified. PR-URL: #13570Fixes: #12248Fixes: #13562 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Jason Ginchereau <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #13620Fixes: #13616 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Minor correction in the comment regarding ssl_set_pkey. PR-URL: #13653 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: James M Snell <[email protected]>
Add common.mustCall() check to test-child-process-stdio-big-write-end. PR-URL: #13605 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Yuta Hiroto <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #13561 Ref: #13452 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Trevor Norris <[email protected]> Reviewed-By: James M Snell <[email protected]>
Add a note to clarify that any platform that is EoL will not be supported by Node.js. PR-URL: #12672Fixes: nodejs/build#688 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Rod Vagg <[email protected]>
Updated onboarding-extras.md to /cc @nodejs/v8-inspector for inspector issues and reorganized /cc list in alphabetical order. PR-URL: #13632Fixes: #13621 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Eugene Ostroukhov <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
* 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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
> Generally it will correspond the name of the resource's constructor. should read "Generally, it will correspond to the name..." PR-URL: #13666Fixes: #13663 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>
In non-buffer tests, change usage of the Buffer constructor to one of the recommended alternatives. PR-URL: #13649 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: Tobias Nießen <[email protected]> Reviewed-By: Brian White <[email protected]>
* 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 <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
* 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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]> Reviewed-By: James M Snell <[email protected]>
- 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: #13650Fixes: #13555Fixes: #13556Fixes: #13562 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Jason Ginchereau <[email protected]> Reviewed-By: Ingvar Stepanyan <[email protected]>
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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
* re-implemented test to parse args instead of post binding (exit 12) * saved failing case as known issue PR-URL: #13373Fixes: #13343 Reviewed-By: James M Snell <[email protected]>
* 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 <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
PR-URL: #13685 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: David Cai <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: James M Snell <[email protected]>
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: #13674Fixes: #13665 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
`max_i` should also include the characters that were just read by `base64_decode_group_slow()`. PR-URL: #13660Fixes: #13636Fixes: #13657 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
* 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 <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: #13673 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #13658 Ref: #13645 (comment) Reviewed-By: João Reis <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Roman Reiss <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
PR-URL: #13413 Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
PR-URL: #13713 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
@addaleax
Copy link
MemberAuthor

@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

@addaleax: ssh [email protected] 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
MemberAuthor

@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

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.

cjihrigand 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 <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Calvin Metcalf <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
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 <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>
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 <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
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 <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
Fix previously-unnoticed typo in `required-modules.js`. Refs: #13758 (comment) PR-URL: #13758 Reviewed-By: Teddy Katz <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]>
* 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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
* 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 <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>
* also update STYLE_GUIDE comment about Em dashes PR-URL: #13749 Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Vse Mozhet Byt <[email protected]>
@addaleax
Copy link
MemberAuthor

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
MemberAuthor

Are we releasing 8.1.3 this week?

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

@refack
Copy link
Contributor

This needs to wait for #13969

@addaleax
Copy link
MemberAuthor

@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 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

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.

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

thanks for the hard work @addaleax, release is done

@mcollina
Copy link
Member

Thanks folks!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

metaIssues and PRs related to the general management of the project.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

20 participants

@addaleax@rvagg@refack@SimenB@tuananh@mcollina@mscdex@nodejs-github-bot@seishun@Trott@XadillaX@thelostone-mc@danbev@gibfahn@sam-github@mutantcornholio@tniessen@mhdawson@vsemozhetbyt@cjihrig