fs: add AbortSignal support to watch by benjamingr · Pull Request #37190 · nodejs/node

benjamingr · 2021-02-02T17:33:32Z

Adds support for AbortSignal in fs.watch

Ref: #37179

cc @nodejs/fs @jasnell

lib/fs.js

nodejs-github-bot · 2021-02-02T20:36:20Z

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

test/parallel/test-fs-watch-abort-signal.js

jasnell · 2021-02-03T14:34:05Z

lib/fs.js

  }
+  if (options.signal) {
+    if (options.signal.aborted) {
+      process.nextTick(() => watcher.close());


Why on next tick?

So that users have a chance to subscribe to the "close" event.

(Just because watch returns an EventEmitter watcher and not a promise or a callback)

jasnell

LGTM with nits fixed

nodejs-github-bot · 2021-02-03T17:27:44Z

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

nodejs-github-bot · 2021-02-04T09:29:11Z

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

benjamingr · 2021-02-04T09:29:13Z

@RaisinTen addressed your comments - any chance this could get a review :]?

aduh95 · 2021-02-04T10:46:13Z

lib/fs.js

+    if (options.signal.aborted) {
+      process.nextTick(() => watcher.close());
+    } else {
+      const listener = () => watcher.close();


nit: you could move the listener declaration above and pass it in process.nextTick to avoid code duplication.

I actually find this way more readable though I don't have very strong feelings about it.

aduh95 · 2021-02-04T10:48:16Z

lib/fs.js

+      process.nextTick(() => watcher.close());
+    } else {
+      const listener = () => watcher.close();
+      options.signal.addEventListener('abort', listener);


Not sure if that matters / is more efficient, but at least it makes it clearer that this is just a one timer:

Suggested change

options.signal.addEventListener('abort', listener);

options.signal.addEventListener('abort', listener, { once: true });

This is a common issue that gets brought up from time to time. The reason I like doing { once: true } less is that it creates the false impression we are relying on the listener being a one-time listener for cleanup.

That assumption has lead to memory leaks in our code before - this way the cleanup is explicit (when the watcher is closed) and I think it makes bugs slightly less likely.

benjamingr · 2021-02-05T10:36:10Z

Landed in 664cce9 🎉

PR-URL: #37190 Refs: #37179 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>

Notable Changes: * crypto: * add keyObject.export() 'jwk' format option (Filip Skokan) #37081 * deps: * upgrade to libuv 1.41.0 (Colin Ihrig) #37360 * doc: * add dmabupt to collaborators (Xu Meng) #37377 * refactor fs docs structure (James M Snell) #37170 * fs: * add fsPromises.watch() (James M Snell) #37179 * use a default callback for fs.close() (James M Snell) #37174 * add AbortSignal support to watch (Benjamin Gruenbaum) #37190 * perf_hooks: * introduce createHistogram (James M Snell) #37155 * stream: * improve Readable.from error handling (Benjamin Gruenbaum) #37158 * timers: * introduce setInterval async iterator (linkgoron) #37153 * tls: * add ability to get cert/peer cert as X509Certificate object (James M Snell) #37070

Notable Changes: * crypto: * add keyObject.export() jwk format option (Filip Skokan) #37081 * deps: * upgrade to libuv 1.41.0 (Colin Ihrig) #37360 * doc: * add dmabupt to collaborators (Xu Meng) #37377 * refactor fs docs structure (James M Snell) #37170 * fs: * add fsPromises.watch() (James M Snell) #37179 * use a default callback for fs.close() (James M Snell) #37174 * add AbortSignal support to watch (Benjamin Gruenbaum) #37190 * perf_hooks: * introduce createHistogram (James M Snell) #37155 * stream: * improve Readable.from error handling (Benjamin Gruenbaum) #37158 * timers: * introduce setInterval async iterator (linkgoron) #37153 * tls: * add ability to get cert/peer cert as X509Certificate object (James M Snell) #37070

PR-URL: #37406 Notable Changes: * crypto: * add keyObject.export() jwk format option (Filip Skokan) #37081 * deps: * upgrade to libuv 1.41.0 (Colin Ihrig) #37360 * doc: * add dmabupt to collaborators (Xu Meng) #37377 * refactor fs docs structure (James M Snell) #37170 * fs: * add fsPromises.watch() (James M Snell) #37179 * use a default callback for fs.close() (James M Snell) #37174 * add AbortSignal support to watch (Benjamin Gruenbaum) #37190 * perf_hooks: * introduce createHistogram (James M Snell) #37155 * stream: * improve Readable.from error handling (Benjamin Gruenbaum) #37158 * timers: * introduce setInterval async iterator (linkgoron) #37153 * tls: * add ability to get cert/peer cert as X509Certificate object (James M Snell) #37070

PR-URL: nodejs#37190 Refs: nodejs#37179 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>

PR-URL: #37190 Backport-PR-URL: #38386 Refs: #37179 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com>

benjamingr added the fs Issues and PRs related to the fs subsystem / file system. label Feb 2, 2021

benjamingr force-pushed the fs-watch-signal branch from bf378d4 to 3365196 Compare February 2, 2021 17:34

aduh95 reviewed Feb 2, 2021

View reviewed changes

lib/fs.js Outdated Show resolved Hide resolved

RaisinTen reviewed Feb 3, 2021

View reviewed changes

test/parallel/test-fs-watch-abort-signal.js Outdated Show resolved Hide resolved

RaisinTen reviewed Feb 3, 2021

View reviewed changes

test/parallel/test-fs-watch-abort-signal.js Outdated Show resolved Hide resolved

jasnell reviewed Feb 3, 2021

View reviewed changes

jasnell approved these changes Feb 3, 2021

View reviewed changes

fs: add AbortSignal support to watch

86d13a0

benjamingr force-pushed the fs-watch-signal branch from c85efae to 86d13a0 Compare February 3, 2021 17:26

aduh95 approved these changes Feb 4, 2021

View reviewed changes

RaisinTen approved these changes Feb 4, 2021

View reviewed changes

benjamingr closed this Feb 5, 2021

benjamingr added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 5, 2021

This was referenced Feb 16, 2021

v15.9.0 proposal #37405

Closed

v15.9.0 proposal #37406

Merged

targos added the backport-open-v14.x label Apr 24, 2021

targos mentioned this pull request Apr 24, 2021

[v14.x] Backport AbortController and friends #38386

Closed

targos added backported-to-v14.x and removed backport-open-v14.x labels Apr 30, 2021

danielleadams mentioned this pull request May 3, 2021

v14.17.0 proposal #38507

Merged

	options.signal.addEventListener('abort', listener);
	options.signal.addEventListener('abort', listener, { once: true });

Uh oh!

Conversation

benjamingr commented Feb 2, 2021

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 2, 2021

Uh oh!

Uh oh!

Uh oh!

jasnell Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

benjamingr Feb 3, 2021

Choose a reason for hiding this comment

Uh oh!

benjamingr Feb 3, 2021 • 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!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Feb 3, 2021

Uh oh!

nodejs-github-bot commented Feb 4, 2021

Uh oh!

benjamingr commented Feb 4, 2021

Uh oh!

aduh95 Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

benjamingr Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

aduh95 Feb 4, 2021 • 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!

benjamingr Feb 4, 2021

Choose a reason for hiding this comment

Uh oh!

benjamingr commented Feb 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

benjamingr Feb 3, 2021 •

edited

Loading

aduh95 Feb 4, 2021 •

edited

Loading