lib: consistent `this` in callbacks by Trott · Pull Request #14645 · nodejs/node · GitHub

This change normalizes a few situations where a callback might be invoked with this set to undefined in one case, but null in a very similar case.

First commit:

test: refactor test-fs-stat

* add `use strict'
* change checks that `this` is mapped to `global` in sloppy mode to
  checks that `this` is `undefined`
* modify arguments to assertions to match docs (actual first, expected
  second)
* add blank line below `common` declaration per test writing guide
* use `assert.ifError()` as appropriate

Second commit:

fs: invoke callbacks with undefined context

Many callbacks appear to be invoked with `this` set to `undefined`
including `fs.stat()`, `fs.lstat()`, and `fs.fstat()`.

However, some such as `fs.open()` and `fs.mkdtemp()` invoke their
callbacks with `this` set to `null`.

Use a consistent way to invoke callbacks to guarantee consistency.

Third commit:

test: check `this` value for `nextTick()`

Depending on how many arguments are provided, `nextTick()` may run its
callback with `this` set to `null` or not. Add assertions for
both cases.

Fourth commit:

process: make `this` value consistent

The value of `this` for callbacks of `nextTick()` can vary depending on
the number of arguments. Make it consistent.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

test process lib fs

Needs a CITGM run and some benchmarks too, although I imagine the consistency aspect overrides anything the benchmarks show unless it's a severe perf hit.

This is the kind of thing we definitely need to benchmark... both here and nextTick() especially because in both scenarios a different type of value is being used with the spread operator (special arguments vs array).

The current next-tick benchmarks do not test the code modified here. I'll add a commit to include this code path in the benchmarks.

There are a few other combinations that should be benchmarked:

cb.bind(undefined)
function (...args) { cb(...args) }
(...args) => cb(...args)

I'll try to check some.

OK, I went back to .apply(undefined, arguments) which probably won't correct as many edge cases, but is better than the current .apply(null, arguments) in that regard and doesn't have a negative perf hit.

@mscdex I updated the process.nextTick() benchmark code in e15b625.

Running the resulting benchmarks shows no significant performance difference.

Command I ran to generate data:

$ node benchmark/compare.js --old ./node-master --new ./node-strictify --filter next-tick --filter args  process | tee compare.csv

Command I ran to analyze it:

$ cat compare.csv | Rscript benchmark/compare.R

Result:

                                              improvement confidence   p.value
 process/next-tick-breadth-args.js millions=2     -0.48 %            0.3246212
 process/next-tick-depth-args.js millions=12       0.42 %            0.1323853

Increasing the number of runs might help decrease the p-values a bit, but so far, it seems like the change does not significantly impact performance.

@nodejs/ctc This is semver-major so needs at least two CTC approvals.

CI: https://ci.nodejs.org/job/node-test-pull-request/9525/

LGTM

There are a few other combinations that should be benchmarked:

cb.bind(undefined)
function (...args) { cb(...args) }
(...args) => cb(...args)

I'll try to check some.

Going to rebase and force push so I can check the fs benchmarks. I don't expect that to turn anything up, but we'll see.

On the nextTick() benchmarks, I can't help but notice that the previous benchmarks tested up to three arguments, and that's exactly how many arguments were fast-pathed by the switch statement. In other words, the implementation and the benchmark matched each other in a way that suggests that we may have been coding to the particulars of the benchmark. It might be worth benchmarking without the switch statement to get an idea of the perf difference now, but that's for another PR.

* add `use strict' * change checks that `this` is mapped to `global` in sloppy mode to checks that `this` is `undefined` * modify arguments to assertions to match docs (actual first, expected second) * add blank line below `common` declaration per test writing guide * use `assert.ifError()` as appropriate

That took longer to run than I wanted, but the benchmark does show one statistically significant regression:

$ cat compare-stream-throughput.csv | Rscript benchmark/compare.R 
                                                             improvement confidence    p.value
 fs/read-stream-throughput.js size=1024 type="asc"                0.21 %            0.83962407
 fs/read-stream-throughput.js size=1024 type="buf"                0.74 %            0.45578639
 fs/read-stream-throughput.js size=1024 type="utf"               -0.83 %            0.36248801
 fs/read-stream-throughput.js size=1048576 type="asc"             0.80 %            0.28382559
 fs/read-stream-throughput.js size=1048576 type="buf"             0.25 %            0.77047635
 fs/read-stream-throughput.js size=1048576 type="utf"             0.36 %            0.45264382
 fs/read-stream-throughput.js size=4096 type="asc"                0.28 %            0.81776035
 fs/read-stream-throughput.js size=4096 type="buf"                0.52 %            0.65622628
 fs/read-stream-throughput.js size=4096 type="utf"                0.87 %            0.24056815
 fs/read-stream-throughput.js size=65535 type="asc"               0.41 %            0.74103616
 fs/read-stream-throughput.js size=65535 type="buf"               0.83 %            0.36577753
 fs/read-stream-throughput.js size=65535 type="utf"               0.16 %            0.72207303
 fs/write-stream-throughput.js size=1024 type="asc" dur=5         0.46 %            0.60467786
 fs/write-stream-throughput.js size=1024 type="buf" dur=5        -0.43 %            0.64367678
 fs/write-stream-throughput.js size=1024 type="utf" dur=5        -0.14 %            0.86030264
 fs/write-stream-throughput.js size=1048576 type="asc" dur=5     -0.51 %            0.24024453
 fs/write-stream-throughput.js size=1048576 type="buf" dur=5     -0.89 %            0.26381972
 fs/write-stream-throughput.js size=1048576 type="utf" dur=5     -0.69 %            0.14121379
 fs/write-stream-throughput.js size=2 type="asc" dur=5            0.01 %            0.99517966
 fs/write-stream-throughput.js size=2 type="buf" dur=5           -0.75 %            0.52078304
 fs/write-stream-throughput.js size=2 type="utf" dur=5            0.23 %            0.84135140
 fs/write-stream-throughput.js size=65535 type="asc" dur=5       -1.90 %          * 0.01707446
 fs/write-stream-throughput.js size=65535 type="buf" dur=5        0.17 %            0.83461519
 fs/write-stream-throughput.js size=65535 type="utf" dur=5       -1.36 %            0.05349453
$

Just to confirm, I reran just that benchmark and sure enough:

$ cat compare-stream-throughput-2.csv | Rscript benchmark/compare.R 
                                                           improvement confidence     p.value
 fs/write-stream-throughput.js size=65535 type="asc" dur=5     -1.99 %         ** 0.009768715
$

I guess I'll see if there's some other way to keep the this value consistent without that perf hit...

Many callbacks appear to be invoked with `this` set to `undefined` including `fs.stat()`, `fs.lstat()`, and `fs.fstat()`. However, some such as `fs.open()` and `fs.mkdtemp()` invoke their callbacks with `this` set to `null`. Change to `undefined`.

Depending on how many arguments are provided, `nextTick()` may run its callback with `this` set to `null` or not. Add assertions for both cases.

The value of `this` for callbacks of `nextTick()` can vary depending on the number of arguments. Make it consistent.

The benchmarks for `process.nextTick()` do not cover the `default` case in the internal code's `switch` statement where the callback receives more than 3 arguments. Modify two of the benchmarks to include this condition.

OK, I went back to .apply(undefined, arguments) which probably won't correct as many edge cases, but is better than the current .apply(null, arguments) in that regard and doesn't have a negative perf hit.

CI: https://ci.nodejs.org/job/node-test-pull-request/9580/

Can one or more previous approvers take a look at 9d6365a and re-affirm their approval? That's the only thing that's changed that hasn't had anyone look at it. Seems pretty unlikely to be controversial to me, but ¯\(ツ)/¯

Still LGTM.

* add `use strict' * change checks that `this` is mapped to `global` in sloppy mode to checks that `this` is `undefined` * modify arguments to assertions to match docs (actual first, expected second) * add blank line below `common` declaration per test writing guide * use `assert.ifError()` as appropriate PR-URL: nodejs#14645 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

Many callbacks appear to be invoked with `this` set to `undefined` including `fs.stat()`, `fs.lstat()`, and `fs.fstat()`. However, some such as `fs.open()` and `fs.mkdtemp()` invoke their callbacks with `this` set to `null`. Change to `undefined`. PR-URL: nodejs#14645 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

Depending on how many arguments are provided, `nextTick()` may run its callback with `this` set to `null` or not. Add assertions for both cases. PR-URL: nodejs#14645 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

The value of `this` for callbacks of `nextTick()` can vary depending on the number of arguments. Make it consistent. PR-URL: nodejs#14645 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

The benchmarks for `process.nextTick()` do not cover the `default` case in the internal code's `switch` statement where the callback receives more than 3 arguments. Modify two of the benchmarks to include this condition. PR-URL: nodejs#14645 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

Landed in c6126b1 2249234 0d3ef5b a253704 97c4394

* **async_hooks** * Older experimental `async_hooks` APIs have been removed [[`d731369b1d`](d731369b1d)] **(SEMVER-MAJOR)** [#14414](#14414) * **Errors** * Multiple built in modules have been migrated to use static error codes * **Domains** * The long deprecated `.dispose()` method has been removed [[`602fd36d95`](602fd36d95)] **(SEMVER-MAJOR)** [#15412](#15412) * **File system** * `fs.ReadStream` and `fs.WriteStream` now make use of `.destroy()` [[`e5c290bed9`](e5c290bed9)] **(SEMVER-MAJOR)** [#15407](#15407) * `fs` callbacks are now invoked with an undefined `this` context [[`2249234fee`](2249234fee)] **(SEMVER-MAJOR)** [#14645](#14645) * **HTTP** * Socket timeout is set when the socket connects [[`10be20a0e8`](10be20a0e8)] **(SEMVER-MAJOR)** [#8895](#8895) * A bug causing the request `error` event to fire twice has been fixed [[`620ba41694`](620ba41694)] **(SEMVER-MAJOR)** [#14659](#14659) * The `pipe` method on `OutgoingMessage` has been disabled [[`156549d8ff`](156549d8ff)] **(SEMVER-MAJOR)** [#14358](#14358) * **HTTP/2** * The `--expose-http2` command-line argument is no longer required [[`f55ee6e24a`](f55ee6e24a)] **(SEMVER-MAJOR)** [#15535](#15535) * **Internationalization** * The `Intl.v8BreakIterator` class has been removed [[`668ad44922`](668ad44922)] **(SEMVER-MAJOR)** [#15238](#15238) * **OS** * `os.EOL` is now read-only [[`f6caeb9526`](f6caeb9526)] **(SEMVER-MAJOR)** [#14622](#14622) * **Process** * It is now possible to pass additional flags to `dlopen` [[`5f22375922`](5f22375922)] **(SEMVER-MAJOR)** [#12794](#12794) * **Timers** * Using a timeout duration larger than 32-bits will now emit a warning [[`ce3586da31`](ce3586da31)] **(SEMVER-MAJOR)** [#15627](#15627) * **TLS** * `parseCertString` has been deprecated [[`468110b327`](468110b327)] **(SEMVER-MAJOR)** [#14249](#14249) * Type-checking for `key`, `cert`, and `ca` options has been added [[`a7dccd040d`](a7dccd040d)] **(SEMVER-MAJOR)** [#14807](#14807)

nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. process Issues and PRs related to the process subsystem. labels Aug 6, 2017

mscdex reviewed Aug 6, 2017

View reviewed changes

lpinca approved these changes Aug 6, 2017

View reviewed changes

Trott added the ctc-review label Aug 7, 2017

mcollina approved these changes Aug 7, 2017

View reviewed changes

Trott force-pushed the strictify branch from e15b625 to 9d03841 Compare August 7, 2017 23:00

jasnell approved these changes Aug 8, 2017

View reviewed changes

Trott removed the ctc-review label Aug 8, 2017

BridgeAR approved these changes Aug 8, 2017

View reviewed changes

refack approved these changes Aug 8, 2017

View reviewed changes

Trott force-pushed the strictify branch from 9d03841 to 87ec89c Compare August 9, 2017 16:30

Trott force-pushed the strictify branch from 87ec89c to 255f8cd Compare August 9, 2017 23:14

Trott added 4 commits August 9, 2017 16:15

test: check this value for nextTick()

23918c4

Depending on how many arguments are provided, `nextTick()` may run its callback with `this` set to `null` or not. Add assertions for both cases.

process: make this value consistent

aa07e87

The value of `this` for callbacks of `nextTick()` can vary depending on the number of arguments. Make it consistent.

benchmark: cover more nextTick() code

64e1f00

The benchmarks for `process.nextTick()` do not cover the `default` case in the internal code's `switch` statement where the callback receives more than 3 arguments. Modify two of the benchmarks to include this condition.

Trott force-pushed the strictify branch from 255f8cd to 64e1f00 Compare August 9, 2017 23:15

Trott closed this Aug 10, 2017

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

Trott deleted the strictify branch July 31, 2018 15:37

lib: consistent `this` in callbacks by Trott · Pull Request #14645 · nodejs/node · GitHub

Uh oh!

Uh oh!

lib: consistent this in callbacks - #14645

lib: consistent this in callbacks#14645Trott wants to merge 5 commits intonodejs:masternodejs/node:masterfrom Trott:strictifyTrott/io.js:strictifyCopy head branch name to clipboard

Conversation

Trott commented Aug 6, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Trott commented Aug 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mscdex Aug 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Trott Aug 6, 2017

Choose a reason for hiding this comment

Uh oh!

refack Aug 8, 2017

Choose a reason for hiding this comment

Uh oh!

Trott Aug 9, 2017

Choose a reason for hiding this comment

Uh oh!

Trott commented Aug 6, 2017

Uh oh!

Trott commented Aug 7, 2017

Uh oh!

Trott commented Aug 7, 2017

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

refack Aug 8, 2017

Choose a reason for hiding this comment

Uh oh!

Trott commented Aug 9, 2017

Uh oh!

Trott commented Aug 9, 2017

Uh oh!

Trott commented Aug 9, 2017

Uh oh!

Trott commented Aug 9, 2017

Uh oh!

Trott commented Aug 10, 2017

Uh oh!

Trott commented Aug 10, 2017

Uh oh!

lpinca commented Aug 10, 2017

Uh oh!

Trott commented Aug 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

lib: consistent this in callbacks#14645
Trott wants to merge 5 commits into
nodejs:masterfrom
Trott:strictify

Trott commented Aug 6, 2017 •
edited
Loading

mscdex Aug 6, 2017 •
edited
Loading