Skip to content

fs: improve fs.watch ENOSPC error message#21846

Closed
addaleax wants to merge 3 commits intonodejs:masterfrom
addaleax:fs-watch-enospc
Closed

fs: improve fs.watch ENOSPC error message#21846
addaleax wants to merge 3 commits intonodejs:masterfrom
addaleax:fs-watch-enospc

Conversation

@addaleax
Copy link
Member

@addaleax addaleax commented Jul 17, 2018

Providing No space left on device is misleading in this case.
Replace it with something that describes it more accurately.

Refs: https://stackoverflow.com/questions/22475849/node-js-error-enospc/32600959

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

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. fs Issues and PRs related to the fs subsystem / file system. labels Jul 17, 2018
@addaleax
Copy link
Member Author

addaleax commented Jul 17, 2018

The test probably needs some per-platform tweaking, so:

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

@addaleax addaleax added the wip Issues and PRs that are still a work in progress. label Jul 17, 2018
@addaleax
Copy link
Member Author

addaleax commented Jul 18, 2018 •
edited
Loading

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

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

lpinca
lpinca approved these changes Jul 18, 2018
View reviewed changes
test/sequential/test-fs-watch-system-limit.js Outdated

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

@addaleax addaleax force-pushed the fs-watch-enospc branch 2 times, most recently from 6f4bd38 to b156539 Compare August 1, 2018 16:16
@addaleax addaleax removed the wip Issues and PRs that are still a work in progress. label Aug 1, 2018
@addaleax
Copy link
Member Author

addaleax commented Aug 1, 2018

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

Might be good to get another @nodejs/tsc review – this error comes with a code already, so I don’t think this has to be semver-major.

targos
targos approved these changes Aug 1, 2018
View reviewed changes
test/sequential/test-fs-watch-system-limit.js Outdated
Copy link
Member

@targos targos Aug 1, 2018

Choose a reason for hiding this comment

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

TBD (to be deleted)

lib/internal/fs/watchers.js Outdated
Copy link
Member

@targos targos Aug 1, 2018

Choose a reason for hiding this comment

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

Nit: Might want to call it message?

Copy link
Member

@joyeecheung joyeecheung Aug 1, 2018

Choose a reason for hiding this comment

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

+1 to call it message. By the way if the key is message it currently will not appear in the final error object as an additional property whereas description will.

Copy link
Member

@jasnell jasnell Aug 12, 2018

Choose a reason for hiding this comment

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

ping @addaleax

@jasnell
Copy link
Member

jasnell commented Sep 10, 2018

Ping again @addaleax

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Sep 10, 2018
@addaleax
fs: improve fs.watch ENOSPC error message
73f4325 
Providing `No space left on device` is misleading in this case.
Replace it with something that describes it more accurately.

Refs: https://stackoverflow.com/questions/22475849/node-js-error-enospc/32600959
@addaleax addaleax removed the stalled Issues and PRs that are stalled. label Sep 11, 2018
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 11, 2018
@addaleax
Copy link
Member Author

addaleax commented Sep 11, 2018

Rebased, with comments addressed.

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

@targos
Copy link
Member

targos commented Sep 16, 2018

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

@addaleax
Copy link
Member Author

addaleax commented Sep 17, 2018

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

@joyeecheung joyeecheung mentioned this pull request Sep 18, 2018
Closed
@addaleax
Copy link
Member Author

addaleax commented Sep 22, 2018

Arm re-build: https://ci.nodejs.org/job/node-test-commit-arm-fanned/3689/

@addaleax
Copy link
Member Author

addaleax commented Sep 22, 2018

… and again: https://ci.nodejs.org/job/node-test-commit-arm-fanned/3692/

@addaleax
Copy link
Member Author

addaleax commented Sep 22, 2018

Landed in 13245dc

@addaleax addaleax closed this Sep 22, 2018
@addaleax addaleax deleted the fs-watch-enospc branch September 22, 2018 15:47
addaleax added a commit that referenced this pull request Sep 22, 2018
@addaleax
fs: improve fs.watch ENOSPC error message
13245dc 
Providing `No space left on device` is misleading in this case.
Replace it with something that describes it more accurately.

Refs: https://stackoverflow.com/questions/22475849/node-js-error-enospc/32600959

PR-URL: #21846
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
targos pushed a commit that referenced this pull request Sep 23, 2018
@addaleax @targos
fs: improve fs.watch ENOSPC error message
c29734c 
Providing `No space left on device` is misleading in this case.
Replace it with something that describes it more accurately.

Refs: https://stackoverflow.com/questions/22475849/node-js-error-enospc/32600959

PR-URL: #21846
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
@targos targos mentioned this pull request Oct 7, 2018
Merged
This was referenced Oct 10, 2018
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@joyeecheung joyeecheung joyeecheung left review comments

@targos targos targos approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

+1 more reviewer

@thefourtheye thefourtheye thefourtheye approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. errors Issues and PRs related to JavaScript errors originated in Node.js core. fs Issues and PRs related to the fs subsystem / file system.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@addaleax @nodejs-github-bot @jasnell @targos @thefourtheye @lpinca @joyeecheung @BridgeAR