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

lib: consistent this in callbacks #14645

Closed
wants to merge 5 commits into from
Closed

lib: consistent this in callbacks #14645

wants to merge 5 commits into from

Conversation

@Trott
Copy link
Member

@Trott Trott commented Aug 6, 2017

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

@Trott
Copy link
Member Author

@Trott Trott commented Aug 6, 2017

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.

Loading

lib/fs.js Outdated
@@ -132,7 +132,7 @@ function makeCallback(cb) {
}

return function() {
return cb.apply(null, arguments);
return cb(...arguments);
Copy link
Contributor

@mscdex mscdex Aug 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Loading

Copy link
Member Author

@Trott Trott Aug 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member

@refack refack Aug 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few other combinations that should be benchmarked:

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

I'll try to check some.

Loading

Copy link
Member Author

@Trott Trott Aug 9, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

@Trott
Copy link
Member Author

@Trott Trott commented Aug 6, 2017

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

Loading

lpinca
lpinca approved these changes Aug 6, 2017
@Trott
Copy link
Member Author

@Trott Trott commented Aug 7, 2017

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

Loading

@Trott
Copy link
Member Author

@Trott Trott commented Aug 7, 2017

Loading

Copy link
Member

@mcollina mcollina left a comment

LGTM

Loading

jasnell
jasnell approved these changes Aug 8, 2017
@Trott Trott removed the ctc-review label Aug 8, 2017
refack
refack approved these changes Aug 8, 2017
lib/fs.js Outdated
@@ -132,7 +132,7 @@ function makeCallback(cb) {
}

return function() {
return cb.apply(null, arguments);
return cb(...arguments);
Copy link
Member

@refack refack Aug 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few other combinations that should be benchmarked:

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

I'll try to check some.

Loading

@Trott
Copy link
Member Author

@Trott Trott commented Aug 9, 2017

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.

Loading

@Trott
Copy link
Member Author

@Trott Trott commented Aug 9, 2017

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.

Loading

* 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
@Trott
Copy link
Member Author

@Trott Trott commented Aug 9, 2017

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

Loading

Trott added 4 commits Aug 9, 2017
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.
@Trott
Copy link
Member Author

@Trott Trott commented Aug 9, 2017

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.

Loading

@Trott
Copy link
Member Author

@Trott Trott commented Aug 10, 2017

Loading

@Trott
Copy link
Member Author

@Trott Trott commented Aug 10, 2017

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 ¯\(ツ)

Loading

@lpinca
Copy link
Member

@lpinca lpinca commented Aug 10, 2017

Still LGTM.

Loading

Trott added a commit to Trott/io.js that referenced this issue Aug 10, 2017
* 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>
Trott added a commit to Trott/io.js that referenced this issue Aug 10, 2017
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>
Trott added a commit to Trott/io.js that referenced this issue Aug 10, 2017
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>
Trott added a commit to Trott/io.js that referenced this issue Aug 10, 2017
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>
Trott added a commit to Trott/io.js that referenced this issue Aug 10, 2017
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>
@Trott
Copy link
Member Author

@Trott Trott commented Aug 10, 2017

Loading

@Trott Trott closed this Aug 10, 2017
jasnell added a commit that referenced this issue Oct 17, 2017
* **async_hooks**
  * Older experimental `async_hooks` APIs have been removed
    [[`d731369b1d`](d731369)]
    **(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`](602fd36)]
    **(SEMVER-MAJOR)** [#15412](#15412)

* **File system**
  * `fs.ReadStream` and `fs.WriteStream` now make use of `.destroy()`
    [[`e5c290bed9`](e5c290b)]
    **(SEMVER-MAJOR)** [#15407](#15407)
  * `fs` callbacks are now invoked with an undefined `this` context
    [[`2249234fee`](2249234)]
    **(SEMVER-MAJOR)** [#14645](#14645)

* **HTTP**
  * Socket timeout is set when the socket connects
    [[`10be20a0e8`](10be20a)]
    **(SEMVER-MAJOR)** [#8895](#8895)
  * A bug causing the request `error` event to fire twice has been fixed
    [[`620ba41694`](620ba41)]
    **(SEMVER-MAJOR)** [#14659](#14659)
  * The `pipe` method on `OutgoingMessage` has been disabled
    [[`156549d8ff`](156549d)]
    **(SEMVER-MAJOR)** [#14358](#14358)

* **HTTP/2**
  * The `--expose-http2` command-line argument is no longer required
    [[`f55ee6e24a`](f55ee6e)]
    **(SEMVER-MAJOR)** [#15535](#15535)

* **Internationalization**
  * The `Intl.v8BreakIterator` class has been removed
    [[`668ad44922`](668ad44)]
    **(SEMVER-MAJOR)** [#15238](#15238)

* **OS**
  * `os.EOL` is now read-only
    [[`f6caeb9526`](f6caeb9)]
    **(SEMVER-MAJOR)** [#14622](#14622)

* **Process**
  * It is now possible to pass additional flags to `dlopen`
    [[`5f22375922`](5f22375)]
    **(SEMVER-MAJOR)** [#12794](#12794)

* **Timers**
  * Using a timeout duration larger than 32-bits will now emit a warning
    [[`ce3586da31`](ce3586d)]
    **(SEMVER-MAJOR)** [#15627](#15627)

* **TLS**
  * `parseCertString` has been deprecated
    [[`468110b327`](468110b)]
    **(SEMVER-MAJOR)** [#14249](#14249)
  * Type-checking for `key`, `cert`, and `ca` options has been added
    [[`a7dccd040d`](a7dccd0)]
    **(SEMVER-MAJOR)** [#14807](#14807)
jasnell added a commit that referenced this issue Oct 29, 2017
* **async_hooks**
  * Older experimental `async_hooks` APIs have been removed
    [[`d731369b1d`](d731369)]
    **(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`](602fd36)]
    **(SEMVER-MAJOR)** [#15412](#15412)

* **File system**
  * `fs.ReadStream` and `fs.WriteStream` now make use of `.destroy()`
    [[`e5c290bed9`](e5c290b)]
    **(SEMVER-MAJOR)** [#15407](#15407)
  * `fs` callbacks are now invoked with an undefined `this` context
    [[`2249234fee`](2249234)]
    **(SEMVER-MAJOR)** [#14645](#14645)

* **HTTP**
  * Socket timeout is set when the socket connects
    [[`10be20a0e8`](10be20a)]
    **(SEMVER-MAJOR)** [#8895](#8895)
  * A bug causing the request `error` event to fire twice has been fixed
    [[`620ba41694`](620ba41)]
    **(SEMVER-MAJOR)** [#14659](#14659)
  * The `pipe` method on `OutgoingMessage` has been disabled
    [[`156549d8ff`](156549d)]
    **(SEMVER-MAJOR)** [#14358](#14358)

* **HTTP/2**
  * The `--expose-http2` command-line argument is no longer required
    [[`f55ee6e24a`](f55ee6e)]
    **(SEMVER-MAJOR)** [#15535](#15535)

* **Internationalization**
  * The `Intl.v8BreakIterator` class has been removed
    [[`668ad44922`](668ad44)]
    **(SEMVER-MAJOR)** [#15238](#15238)

* **OS**
  * `os.EOL` is now read-only
    [[`f6caeb9526`](f6caeb9)]
    **(SEMVER-MAJOR)** [#14622](#14622)

* **Process**
  * It is now possible to pass additional flags to `dlopen`
    [[`5f22375922`](5f22375)]
    **(SEMVER-MAJOR)** [#12794](#12794)

* **Timers**
  * Using a timeout duration larger than 32-bits will now emit a warning
    [[`ce3586da31`](ce3586d)]
    **(SEMVER-MAJOR)** [#15627](#15627)

* **TLS**
  * `parseCertString` has been deprecated
    [[`468110b327`](468110b)]
    **(SEMVER-MAJOR)** [#14249](#14249)
  * Type-checking for `key`, `cert`, and `ca` options has been added
    [[`a7dccd040d`](a7dccd0)]
    **(SEMVER-MAJOR)** [#14807](#14807)
jasnell added a commit that referenced this issue Oct 29, 2017
* **async_hooks**
  * Older experimental `async_hooks` APIs have been removed
    [[`d731369b1d`](d731369)]
    **(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`](602fd36)]
    **(SEMVER-MAJOR)** [#15412](#15412)

* **File system**
  * `fs.ReadStream` and `fs.WriteStream` now make use of `.destroy()`
    [[`e5c290bed9`](e5c290b)]
    **(SEMVER-MAJOR)** [#15407](#15407)
  * `fs` callbacks are now invoked with an undefined `this` context
    [[`2249234fee`](2249234)]
    **(SEMVER-MAJOR)** [#14645](#14645)

* **HTTP**
  * Socket timeout is set when the socket connects
    [[`10be20a0e8`](10be20a)]
    **(SEMVER-MAJOR)** [#8895](#8895)
  * A bug causing the request `error` event to fire twice has been fixed
    [[`620ba41694`](620ba41)]
    **(SEMVER-MAJOR)** [#14659](#14659)
  * The `pipe` method on `OutgoingMessage` has been disabled
    [[`156549d8ff`](156549d)]
    **(SEMVER-MAJOR)** [#14358](#14358)

* **HTTP/2**
  * The `--expose-http2` command-line argument is no longer required
    [[`f55ee6e24a`](f55ee6e)]
    **(SEMVER-MAJOR)** [#15535](#15535)

* **Internationalization**
  * The `Intl.v8BreakIterator` class has been removed
    [[`668ad44922`](668ad44)]
    **(SEMVER-MAJOR)** [#15238](#15238)

* **OS**
  * `os.EOL` is now read-only
    [[`f6caeb9526`](f6caeb9)]
    **(SEMVER-MAJOR)** [#14622](#14622)

* **Process**
  * It is now possible to pass additional flags to `dlopen`
    [[`5f22375922`](5f22375)]
    **(SEMVER-MAJOR)** [#12794](#12794)

* **Timers**
  * Using a timeout duration larger than 32-bits will now emit a warning
    [[`ce3586da31`](ce3586d)]
    **(SEMVER-MAJOR)** [#15627](#15627)

* **TLS**
  * `parseCertString` has been deprecated
    [[`468110b327`](468110b)]
    **(SEMVER-MAJOR)** [#14249](#14249)
  * Type-checking for `key`, `cert`, and `ca` options has been added
    [[`a7dccd040d`](a7dccd0)]
    **(SEMVER-MAJOR)** [#14807](#14807)
jasnell added a commit that referenced this issue Oct 29, 2017
* **async_hooks**
  * Older experimental `async_hooks` APIs have been removed
    [[`d731369b1d`](d731369)]
    **(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`](602fd36)]
    **(SEMVER-MAJOR)** [#15412](#15412)

* **File system**
  * `fs.ReadStream` and `fs.WriteStream` now make use of `.destroy()`
    [[`e5c290bed9`](e5c290b)]
    **(SEMVER-MAJOR)** [#15407](#15407)
  * `fs` callbacks are now invoked with an undefined `this` context
    [[`2249234fee`](2249234)]
    **(SEMVER-MAJOR)** [#14645](#14645)

* **HTTP**
  * Socket timeout is set when the socket connects
    [[`10be20a0e8`](10be20a)]
    **(SEMVER-MAJOR)** [#8895](#8895)
  * A bug causing the request `error` event to fire twice has been fixed
    [[`620ba41694`](620ba41)]
    **(SEMVER-MAJOR)** [#14659](#14659)
  * The `pipe` method on `OutgoingMessage` has been disabled
    [[`156549d8ff`](156549d)]
    **(SEMVER-MAJOR)** [#14358](#14358)

* **HTTP/2**
  * The `--expose-http2` command-line argument is no longer required
    [[`f55ee6e24a`](f55ee6e)]
    **(SEMVER-MAJOR)** [#15535](#15535)

* **Internationalization**
  * The `Intl.v8BreakIterator` class has been removed
    [[`668ad44922`](668ad44)]
    **(SEMVER-MAJOR)** [#15238](#15238)

* **OS**
  * `os.EOL` is now read-only
    [[`f6caeb9526`](f6caeb9)]
    **(SEMVER-MAJOR)** [#14622](#14622)

* **Process**
  * It is now possible to pass additional flags to `dlopen`
    [[`5f22375922`](5f22375)]
    **(SEMVER-MAJOR)** [#12794](#12794)

* **Timers**
  * Using a timeout duration larger than 32-bits will now emit a warning
    [[`ce3586da31`](ce3586d)]
    **(SEMVER-MAJOR)** [#15627](#15627)

* **TLS**
  * `parseCertString` has been deprecated
    [[`468110b327`](468110b)]
    **(SEMVER-MAJOR)** [#14249](#14249)
  * Type-checking for `key`, `cert`, and `ca` options has been added
    [[`a7dccd040d`](a7dccd0)]
    **(SEMVER-MAJOR)** [#14807](#14807)
@Trott Trott deleted the strictify branch Jul 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants