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

v6.8.1 proposal #9104

Merged
merged 5 commits into from
Oct 15, 2016
Merged

v6.8.1 proposal #9104

merged 5 commits into from
Oct 15, 2016

Conversation

evanlucas
Copy link
Contributor

@evanlucas evanlucas commented Oct 14, 2016

2016-10-14, Version 6.8.1 (Current), @evanlucas

Notable changes

  • build: Fix building with shared zlib. (Bradley T. Hughes) #9077
  • stream: Fix regression in stream.Writable subclass instanceof checks. (Anna Henningsen) #9088
  • timers: Fix regression where immediates that are cleared in the callback would never be called. (Brian White) #9086

Commits

  • [8d2206fe41] - build: add -DZLIB_CONST when building with --shared-zlib (Bradley T. Hughes) #9077
  • [8c4fab0a28] - stream: fix Writable subclass instanceof checks (Anna Henningsen) #9088
  • [7171bd6311] - timers: fix regression with clearImmediate() (Brian White) #9086

mscdex and others added 2 commits October 14, 2016 16:29
This commit fixes a regression introduced in 0ed8839 that caused
additional queued immediate callbacks to be ignored if
`clearImmediate(immediate)` was called within the callback for
`immediate`.

PR-URL: #9086
Fixes: #9084
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
2a4b068 introduced a regression in where checking
`instanceof` would fail for `Writable` subclasses inside the
subclass constructor, i.e. before `Writable()` was called.

Also, calling `null instanceof Writable` or
`undefined instanceof Writable` would fail due to accessing the
`_writableState` property of the target object.

This fixes these problems.

PR-URL: #9088
Ref: #8834 (comment)
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
@nodejs-github-bot nodejs-github-bot added meta Issues and PRs related to the general management of the project. stream Issues and PRs related to the stream subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. v6.x labels Oct 14, 2016
@evanlucas
Copy link
Contributor Author

/cc @nodejs/ctc @nodejs/release does anyone object to this?

@mscdex
Copy link
Contributor

mscdex commented Oct 14, 2016

@thealphanerd had mentioned possibly also including #9077 that fixes a build issue on ArchLinux.

@evanlucas
Copy link
Contributor Author

ah thanks for the reminder @mscdex!

@Trott
Copy link
Member

Trott commented Oct 14, 2016

does anyone object to this?

I support timely and as-frequent-as-necessary patch releases to fix regressions. 💯 👍

bradleythughes and others added 2 commits October 14, 2016 17:01
Commit 782620f added the define only when building with the bundled
zlib. Using a shared zlib results in build breakage:

../src/inspector_agent.cc:179:16: error: assigning to 'Bytef *' (aka 'unsigned char *') from incompatible type
      'const uint8_t *' (aka 'const unsigned char *')
  strm.next_in = PROTOCOL_JSON + 3;
               ^ ~~~~~~~~~~~~~~~~~
1 error generated.

PR-URL: #9077
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
* build: Fix building with shared zlib. (Bradley T. Hughes) [#9077](#9077)
* stream: fix `Writable` subclass instanceof checks (Anna Henningsen) [#9088](#9088)
* timers: fix regression with clearImmediate() (Brian White) [#9086](#9086)

PR-URL: #9104
@evanlucas
Copy link
Contributor Author

so, there are some freebsd failures that I'm not sure if they are related to the timers fix in here or not? https://ci.nodejs.org/job/node-test-commit-freebsd/4828/nodes=freebsd10-64/tapResults/

Both of these are failing pretty consistently:

  • parallel/test-timers-blocking-callback
  • parallel/test-timers-same-timeout-wrong-list-deleted

@misterdjules
Copy link

@evanlucas Looking at those failures.

@mscdex
Copy link
Contributor

mscdex commented Oct 15, 2016

@evanlucas They're known flaky tests I believe. See #8949, #8041, and #7929.

I just ran a PR through CI and it came back all green (including freebsd).

@misterdjules
Copy link

parallel/test-timers-blocking-callback is definitely flaky by design, my apologies again. #8041 tracks this problem.

parallel/test-timers-same-timeout-wrong-list-deleted is also flaky by design, there's at least one existing issue about its flakyness at #8459.

@evanlucas
Copy link
Contributor Author

Ah ok, I feel better about it then. Thanks y'all. I've already got the binaries built so i'll go ahead and promote them

@evanlucas evanlucas merged commit 4613c22 into v6.x Oct 15, 2016
@evanlucas evanlucas deleted the v6.8.1-proposal branch October 15, 2016 00:35
evanlucas added a commit that referenced this pull request Oct 15, 2016
* build: Fix building with shared zlib. (Bradley T. Hughes) [#9077](#9077)
* stream: fix `Writable` subclass instanceof checks (Anna Henningsen) [#9088](#9088)
* timers: fix regression with clearImmediate() (Brian White) [#9086](#9086)

PR-URL: #9104
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. stream Issues and PRs related to the stream subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants