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.

Sign up for GitHub

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

Jump to bottom

V4.3.1 proposal #5200

Merged
merged 64 commits into from
Feb 17, 2016
Merged

V4.3.1 proposal #5200

merged 64 commits into from
Feb 17, 2016

Conversation

MylesBorins
Copy link
Contributor

Notable changes

  • buffer
    • make byteLength work with Buffer correctly (Jackson Tian)
  • debugger
    • guard against call from non-node context (Ben Noordhuis)
      • #4328
      • fixes segfaults in debugger
    • do not incept debug context (Myles Borins)
      • #4815
      • fixes crash in debugger when using util methods
  • deps
    • update to http-parser 2.5.2 (James Snell)

Commits

@MylesBorins MylesBorins force-pushed the v4.3.1-proposal branch 2 times, most recently from 5bdfd82 to b46ee1d Compare February 11, 2016 23:54
@MylesBorins
Copy link
Contributor Author

CI: https://ci.nodejs.org/job/node-test-pull-request/1642/
citgm: https://ci.nodejs.org/job/thealphanerd-smoker/69/

edit: ARM CI failure is known flaky test. Fix should land in next release
#5125

edit: citgm is green! (known failure on ppc)

@mscdex mscdex added the meta Issues and PRs related to the general management of the project. label Feb 12, 2016
@MylesBorins
Copy link
Contributor Author

Build Job for rc1: https://ci.nodejs.org/job/iojs+release/393/

@jasnell
Copy link
Member

jasnell commented Feb 12, 2016

LGTM if CI is green.
@nodejs/lts @nodejs/collaborators ... the plan is to start a regular Release Candidate cycle for all regular LTS releases. The 4.3.1-rc.1 should be available soon with the actual release expected on Tuesday of next week if everything looks good. For the upcoming minor bump to 4.4, we'll do a RC cycle of at least a week before cutting the actual release. To avoid regressions and to make sure these releases are stable, we need as many people as possible to test the RC's.

@MylesBorins
Copy link
Contributor Author

Failures on windows

custom_actions.vcxproj -> c:\workspace\iojs+release\nodes\win2008r2-release-ia32\tools\msvs\msi\Release\custom_actions.dll
light.exe : error LGHT0103: The system cannot find the file '..\..\..\deps\npm\\node_modules\npm-install-checks\node_modules\npmlog\node_modules\gauge\node_modules\lodash.padright\node_modules\lodash._createpadding\node_modules\lodash.repeat\package.json' with type ''. [c:\workspace\iojs+release\nodes\win2008r2-release-ia32\tools\msvs\msi\nodemsi.wixproj]
Build step 'Conditional steps (multiple)' marked build as failure

Digging into npm tree changes

@jasnell
Copy link
Member

jasnell commented Feb 12, 2016

oy.. I think this has come up before. /cc @zkat

@zkat
Copy link
Contributor

zkat commented Feb 12, 2016

Yeaaaaah. It's tricky cause we can't just flatten the tree -- I'll talk with folks tomorrow and see if we can come up with a solution that fixes this without breaking everything else.

@rvagg
Copy link
Member

rvagg commented Feb 12, 2016

  • @thealphanerd would you mind putting PR links in the Notable changes section along with attribution, it just makes it easier to click through to find stuff.
  • [c8cc179] - querystring: improve parse() performance (Brian White) querystring: improve parse() performance #4675 is a little bit of a concern, mainly because that kind of change is so ripe for finding edge cases. We've been through one release cycle with this change in 5.5.0 but I'm still hesitant. This one's not as big as a couple of the other recent perf improvements (mainly where they involve unwinding a regex with a long, complicated function) so it's borderline for me and I think I'd even prefer holding it off in staging for a little longer.
  • [d8fbefc] - buffer: make byteLength work with Buffer correctly (Jackson Tian) buffer: make byteLength work with Buffer correctly #4738 is another borderline for me, it's clearly a bug that needed to be fixed but it's also a breaking change if you happen to be relying on the existing (buggy) way it works. Not an objection for this release, but I'm wondering if we should create another bucket (label or branch) to put these kind of borderline changes in so we can consider them in isolation from the rest.

Lastly, @thealphanerd, how's the cherry-picking going? I'm experiencing a little bit of pain when doing v5.x as we move on but I imagine v4.x-staging is even more difficult.

@MylesBorins
Copy link
Contributor Author

@rvagg

  • I'll update the commit / changelog tomorrow.
  • let's hold c8cc179 back from this release and put it into the next release, which itself will have a longer rc phase.
  • should we make an issue on http://github.com/nodejs/lts?

Lastly, not real problems with cherry-picking tbqh. I wonder if the staging branch has made things a bit simpler?

@jasnell
Copy link
Member

jasnell commented Feb 12, 2016

+1 to being conservative with the perf related fixes. I want to make sure we start doing a RC cycle with every LTS release so we can catch issues faster but we definitely need everyone to help test to make that effective.

I think the cherry-picking in v4.x is going fairly smooth because we are also avoiding the semver-minor bumps.

@MylesBorins
Copy link
Contributor Author

So it looks like there are a handful of things going on with npm

4.2.4 had a similar issue but managed to avoid it via deduping

The same dedupe is not working anymore.

These problems are present in npm-install-check... specifically in the tree of gauge, which is no longer deduping specific lodash dependencies.

I have sent a PR over to gauge that updates the lodash dependency. The updated lodash modules have significantly shallower trees, doing away with the two modules that are being repeated and not deduping. This alone may fix the problem.

It also appears that npm-install-checks and node-gyp are relying on older / incompatible versions of npm-log... which may be adding to the weirdness.

At this point I am ready to revert the npm updates so that we can get and RC out... but I would really like to avoid that as there are some bugs that are being fixed by the npm update

/cc @nodejs/npm

@MylesBorins
Copy link
Contributor Author

It looks like there is an open PR to update npmlog in node-gyp. @rvagg can you escalate this?

nodejs/node-gyp#861

@MylesBorins
Copy link
Contributor Author

It looks like npm-install-checks that is present in npm v2 is two versions behind. The latest version doesn't even have npmlog as a dep anymore. I'm going to experiment with updating that dep and seeing if that helps

edit: talked to @zkat version 2 - 3 do not work with npm@2. It might be possible to release a new version of the 1.x line with an updated npmlog

@MylesBorins MylesBorins force-pushed the v4.3.1-proposal branch 3 times, most recently from c172a6a to 4a4661a Compare February 13, 2016 01:33
@MylesBorins
Copy link
Contributor Author

So i've gone through and removed the updates to npm for now. I've also backed out c8cc179 as requested by @rvagg

ci: https://ci.nodejs.org/job/node-test-pull-request/1648/
citgm: https://ci.nodejs.org/job/thealphanerd-smoker/71/

edit:
CI failure on arm is expected #4830
CI failures on linux are all infra related

edit:
citgm failure on fedora21 appears to be infra related, single failing test due to timing out in the Jade test suite.

@rvagg
Copy link
Member

rvagg commented Feb 13, 2016

fwiw, working on getting a semver-minor update of node-gyp out in the next few days with that npmlog fix plus hopefully nodejs/node-gyp#877 and nodejs/node-gyp#878.

@MylesBorins
Copy link
Contributor Author

rc2 build in progress: https://ci.nodejs.org/job/iojs+release/397/

@jasnell
Copy link
Member

jasnell commented Feb 13, 2016

@nodejs/collaborators ... just a heads up... please help us test this release candidate for v4.3.1 : https://nodejs.org/download/rc/v4.3.1-rc.2/

@mscdex mscdex mentioned this pull request Feb 13, 2016
Closed
@mikemorris
Copy link

Hey, I'd like to lobby for including #4482 in this release. We're hitting the idle socket timeout this commit fixes, and currently have to cherry pick this commit into Node v4 and build from source as a workaround.

@MylesBorins
Copy link
Contributor Author

@mikemorris this commit will be coming very soon in v4.4.0
You should expect an RC with those updates in the next 24 - 48 hours.

We are trying to get 4.3 as stable as possible with this update and then as quickly as possible get out a release with a few semver minor addition, including #4482

bnoordhuis and others added 3 commits February 15, 2016 11:30
@bnoordhuis
src: don't check failure with ERR_peek_error()
c41ed59 
It's possible there is already an existing error on OpenSSL's error
stack that is unrelated to the EVP_DigestInit_ex() operation we just
executed.

Fixes: nodejs#4221
PR-URL: nodejs#4731
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@bnoordhuis
debugger: guard against call from non-node context
667f7a7 
Fix a segmentation fault when the debug message handler was called from
a context without an associated `node::Environment`.

Fixes: nodejs#4261
Fixes: nodejs#4322
PR-URL: nodejs#4328
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@rvagg
tools: add license-builder.sh to construct LICENSE
720b03d 
PR-URL: nodejs#4194
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
kohei-takata and others added 14 commits February 15, 2016 11:30
@kohei-takata
readline: Remove XXX and output debuglog
4fbcb47 
Remove a comment that has a word 'XXX'.
And add a line to output debuglog of error.

PR-URL: nodejs#4690
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@rvagg
doc: update branch-diff arguments in release doc
91066b5 
PR-URL: nodejs#4691
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@pgeiss
buffer: remove unnecessary TODO comments
fb615bd 
Refs: nodejs#4642
PR-URL: nodejs#4719
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@estliberitas
doc: add servername parameter docs
cb5986d 
Add `servername` parameter docs for `https.request()` method.

Follows nodejs#4389

PR-URL: nodejs#4729
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@Trott
test: make test-cluster-disconnect-leak reliable
0178001 
Previously, test-cluster-disconnect-leak had two issues:

* Magic numbers: How many times to spawn a worker was determined through
empirical experimentation. This means that as new platforms and new
CPU/RAM configurations are tested, the magic numbers require more
and more refinement. This brings us to...

* Non-determinism: The test *seems* to fail all the time when the bug
it tests for is present, but it's really a judgment based on sampling.
"Oh, with 8 workers per CPU, it fails about 80% of the time. Let's try
16..."

This revised version of the test takes a different approach. The fix
for the bug that the test was written for means that the `disconnect`
event will fire reliably for a single worker. So we check for that and
the test still fails when the fix is not in the code base and succeeds
when it is.

Advantages of this approach include:

* The test runs much faster.
* The test now works on Windows. The previous version skipped Windows.
* The test should be reliable on any new platform regardless of CPU and
RAM.

Ref: nodejs#4674

PR-URL: nodejs#4736
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@JacksonTian
buffer: make byteLength work with Buffer correctly
748d2b4 
Make the byteLength work correctly when input is Buffer.

e.g:

```js
// The incomplete unicode string
Buffer.byteLength(new Buffer([0xe4, 0xb8, 0xad, 0xe6, 0x96]))
```
The old output: 9
The new output: 5

PR-URL: nodejs#4738
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Trott
test: improve test-cluster-disconnect-suicide-race
be82745 
Previously, test-cluster-disconnect-suicide-race had two issues:

* Magic numbers: How many times to spawn a worker was determined through
empirical experimentation. This means that as new platforms and new
CPU/RAM configurations are tested, the magic numbers require more
and more refinement. This brings us to...

* Non-determinism: The test seems to fail all the time when the bug
it tests for is present, but it's really a judgment based on sampling.
"Oh, with 8 workers per CPU, it fails about 80% of the time. Let's try
16..."

This revised version of the test takes a different approach. The fix
for the bug that the test was written for means that the disconnect
event will fire on a subsequent tick. So we check for that and the test
still fails when the fix is not in the code base and succeeds when it
is.

Advantages of this approach include:

* The test runs much faster.
* The test should be reliable on any new platform regardless of CPU and
RAM.

PR-URL: nodejs#4739
Ref: nodejs#4674
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
@silverwind
doc: style fixes for the TOC
39cb69c 
- Hide the scrollbar on the TOC on all browsers. It was never the
  intention for it to be visible with the scroll indication in place.
  A wrapper element with 20px padding was added to accommodate for
  hopefully all scrollbar widths as well as to avoid overflowing
  content.
- Fixed the scroll indication gradient on Safari, which was caused by
  the wrong from-color, which now matches the to-color.
- Fixed a issue in old IE where the TOC didn't render on the correct
  position through setting `left: 0` and `top: 0` on it.

PR-URL: nodejs#4748
Reviewed-By: James M Snell <jasnell@gmail.com>
@silverwind
test: fix issues for space-in-parens ESLint rule
447347c 
PR-URL: nodejs#4753
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
@silverwind
tools: enable space-in-parens ESLint rule
837ebd1 
Ref: http://eslint.org/docs/rules/space-in-parens.html
PR-URL: nodejs#4753
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
@zkat
doc: update list of personal traits in CoC
c3956d0 
rust-lang/prev.rust-lang.org#288 landed in the Rust repo
so it seems like a good idea to just bring the updated list in.

We also received a request to do this in nodejs/inclusivity#82
so this should resolve that.

Thanks to [@Charlotteis](https://github.com/Charlotteis) for bringing
up the original issue.

Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Myles Borins <mborins@us.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
PR-URL: nodejs#4801
Fixes: nodejs/inclusivity#82
@MylesBorins
tools: add support for subkeys in release tools
b66db49 
Replace grep with awk to add support for subkeys

PR-URL: nodejs#4807
Reviewed-By: Rod Vagg <rod@vagg.org>
@santigimeno
test: fix tls-no-rsa-key flakiness
f205e99 
In some conditions it can happen that the client-side socket is destroyed
before the server-side socket has gracefully closed, thus causing a
'ECONNRESET' error in this socket. To solve this, wait in the client-side
socket for the 'end' event before closing it.

PR-URL: nodejs#4043
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
node_contextify: do not incept debug context
1b070e4 
Currently a debug context is created for various calls to util.

If the node debugger is being run the main context is the debug
context. In this case node_contextify was freeing the debug context
and causing everything to explode.

This change moves around the logic and no longer frees the context.

There is a concern about the dangling pointer

The regression test was adapted from code submitted by @3y3 in nodejs#4815

Fixes: nodejs#4440
Fixes: nodejs#4815
Fixes: nodejs#4597
Fixes: nodejs#4952

PR-URL: nodejs#4815

Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@MylesBorins
Copy link
Contributor Author

CI: https://ci.nodejs.org/job/node-test-pull-request/1670/
citgm: https://ci.nodejs.org/job/thealphanerd-smoker/74/

edit: CI failure is infra related
edit: citgm is all green except for expected failure on PPC with eslint

@MylesBorins
Copy link
Contributor Author

rc.3 build: https://ci.nodejs.org/job/iojs+release/398/

@MylesBorins
Copy link
Contributor Author

As we have not had any problems with any of the rc's I am going to move forward with this release in the next two hours.

Last chance to raise any concerns @nodejs/collaborators

2016-02-16, Version 4.3.1 'Argon' (LTS)
ef37a46 
Notable changes:

* buffer: make byteLength work with Buffer correctly (Jackson Tian)
  - nodejs#4738
* debugger: guard against call from non-node context (Ben Noordhuis)
  - nodejs#4328
* node_contextify: do not incept debug context (Myles Borins)
  - nodejs#4819
* deps: update to http-parser 2.5.2 (James Snell)
  - nodejs#5238
@ChALkeR
Copy link
Member

ChALkeR commented Feb 16, 2016

@thealphanerd #5168 is a segfault fix, but it's a rare case and perhaps not worth the rush or delaying the release.

@MylesBorins
Copy link
Contributor Author

@ChALkeR I feel like that commit is still too fresh. I'll make sure to get it in the v4.4.0-rc

@joaocgreis
Copy link
Member

@thealphanerd I shortened the workspace of release machines from c:\workspace\iojs+release\nodes\win2008r2-release-ia32 to c:\ws. That allows rc1 above to build the MSI, but I won't be able to make it shorter if the tree depth of npm grows again.

@MylesBorins
Copy link
Contributor Author

@joaocgreis thanks for getting to that!

I'm going to avoid rolling the npm updates into this release as we are not assured that the path issues are solved after install. I'll roll all those updates into the next RC though!

@MylesBorins
Copy link
Contributor Author

Release Build: https://ci.nodejs.org/job/iojs+release/404/

@MylesBorins MylesBorins merged commit ef37a46 into nodejs:v4.x Feb 17, 2016
MylesBorins pushed a commit that referenced this pull request Feb 17, 2016
Working on v4.3.2
d442286 
PR-URL: #5200 (comment)
MylesBorins pushed a commit that referenced this pull request Feb 17, 2016
2016-02-16, Version 4.3.1 'Argon' (LTS)
c1b3d78 
Notable changes:

* buffer: make byteLength work with Buffer correctly (Jackson Tian)
  - #4738
* debugger: guard against call from non-node context (Ben Noordhuis)
  - #4328
* node_contextify: do not incept debug context (Myles Borins)
  - #4819
* deps: update to http-parser 2.5.2 (James Snell)
  - #5238

PR-URL: #5200 (comment)
@MylesBorins MylesBorins deleted the v4.3.1-proposal branch March 2, 2016 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
meta Issues and PRs related to the general management of the project.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.