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

fs: add AbortSignal support to watch #37190

Closed
wants to merge 1 commit into from
Closed

fs: add AbortSignal support to watch #37190

wants to merge 1 commit into from

Conversation

benjamingr
Copy link
Member

Adds support for AbortSignal in fs.watch

Ref: #37179

cc @nodejs/fs @jasnell

@benjamingr benjamingr added the fs Issues and PRs related to the fs subsystem / file system. label Feb 2, 2021
aduh95
aduh95 reviewed Feb 2, 2021
View reviewed changes
lib/fs.js Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

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

jasnell
jasnell reviewed Feb 3, 2021
View reviewed changes
lib/fs.js
@@ -1579,6 +1579,17 @@ function watch(filename, options, listener) {
if (listener) {
watcher.addListener('change', listener);
}
if (options.signal) {
if (options.signal.aborted) {
process.nextTick(() => watcher.close());
Copy link
Member

Choose a reason for hiding this comment

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

Why on next tick?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@benjamingr benjamingr Feb 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

jasnell
jasnell approved these changes Feb 3, 2021
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM with nits fixed

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@benjamingr
Copy link
Member Author

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

aduh95
aduh95 approved these changes Feb 4, 2021
View reviewed changes
lib/fs.js
if (options.signal.aborted) {
process.nextTick(() => watcher.close());
} else {
const listener = () => watcher.close();
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

lib/fs.js
process.nextTick(() => watcher.close());
} else {
const listener = () => watcher.close();
options.signal.addEventListener('abort', listener);
Copy link
Contributor

@aduh95 aduh95 Feb 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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 });

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Landed in 664cce9 🎉

@benjamingr benjamingr closed this Feb 5, 2021
benjamingr added a commit that referenced this pull request Feb 5, 2021
@benjamingr
fs: add AbortSignal support to watch
664cce9 
PR-URL: #37190
Refs: #37179
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
@benjamingr benjamingr added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 5, 2021
danielleadams pushed a commit that referenced this pull request Feb 16, 2021
@benjamingr @danielleadams
fs: add AbortSignal support to watch
acd087d 
PR-URL: #37190
Refs: #37179
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
danielleadams added a commit that referenced this pull request Feb 16, 2021
@danielleadams
2021-02-16, Version 15.9.0 (Current)
5133659 
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
This was referenced Feb 16, 2021
Closed
Merged
danielleadams added a commit that referenced this pull request Feb 17, 2021
@danielleadams
2021-02-17, Version 15.9.0 (Current)
889ffde 
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
danielleadams added a commit that referenced this pull request Feb 17, 2021
@danielleadams
2021-02-17, Version 15.9.0 (Current)
b471267 
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
danielleadams added a commit that referenced this pull request Feb 17, 2021
@danielleadams
2021-02-17, Version 15.9.0 (Current)
877f66e 
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
danielleadams added a commit that referenced this pull request Feb 18, 2021
@danielleadams
2021-02-17, Version 15.9.0 (Current)
3bf50a7 
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
targos pushed a commit to targos/node that referenced this pull request Apr 24, 2021
@benjamingr @targos
fs: add AbortSignal support to watch
70f1f8d 
PR-URL: nodejs#37190
Refs: nodejs#37179
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
@targos targos mentioned this pull request Apr 24, 2021
Closed
targos pushed a commit to targos/node that referenced this pull request Apr 26, 2021
@benjamingr @targos
fs: add AbortSignal support to watch
dbe5047 
PR-URL: nodejs#37190
Refs: nodejs#37179
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
targos pushed a commit to targos/node that referenced this pull request Apr 30, 2021
@benjamingr @targos
fs: add AbortSignal support to watch
ef6af8c 
PR-URL: nodejs#37190
Refs: nodejs#37179
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
targos pushed a commit that referenced this pull request Apr 30, 2021
@benjamingr @targos
fs: add AbortSignal support to watch
7aa2e5d 
PR-URL: #37190
Backport-PR-URL: #38386
Refs: #37179
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaisinTen RaisinTen RaisinTen approved these changes

@aduh95 aduh95 aduh95 approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@benjamingr @nodejs-github-bot @jasnell @aduh95 @RaisinTen @targos