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

fs: refactor rimraf retry options #30644

Merged
merged 5 commits into from
Nov 27, 2019
Merged

fs: refactor rimraf retry options #30644

merged 5 commits into from
Nov 27, 2019

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Nov 25, 2019

This PR implements the changes proposed in #30580.

This PR doesn't quite close out #30580 though because synchronous retries are not able to be fully implemented just yet (libuv/libuv#2548 should enable that soon though).

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

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Nov 25, 2019
doc/api/fs.md Outdated
100ms longer on each try. This option represents the number of retries. This
option is ignored if the `recursive` option is not `true`. **Default:** `3`.
* `maxRetries` {integer} If an `EBUSY`, `EMFILE`, `ENOTEMPTY`, or `EPERM`
error is encountered, Node.js will retry the operation with a linear backoff
Copy link
Contributor

Choose a reason for hiding this comment

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

How about supporting exponential backoff or allow to custom, which may react to the real world.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't plan on adding that myself, and definitely not in this PR. But someone could open a follow up PR that passes in a user defined function for that.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 27, 2019

ping for reviews

lib/internal/fs/rimraf.js Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 27, 2019

The failures were test-http2-client-upload, which is a known flake, and test-inspector-wait-for-connection, which is #30619. I'm going to land this.

cjihrig and others added 5 commits November 27, 2019 18:27
This is part of reworking the rimraf retry logic.

Refs: nodejs#30580
PR-URL: nodejs#30644
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
This commit makes retries an opt-in feature by defaulting
to no automatic retries. This will be particularly important
once synchronous operations can sleep between attempts.

Refs: nodejs#30580
PR-URL: nodejs#30644
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
This commit removes the emfileWait option. EMFILE errors are
now handled the same as any other retriable error.

Refs: nodejs#30580
PR-URL: nodejs#30644
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
This commit adds a retryDelay option to rimraf which configures
the amount of time between retry operations.

Refs: nodejs#30580
PR-URL: nodejs#30644
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Co-authored-by: Thang Tran <[email protected]>
Fixes: nodejs#30482
Refs: nodejs#30499
Refs: nodejs#30580
PR-URL: nodejs#30644
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@cjihrig
Copy link
Contributor Author

cjihrig commented Nov 27, 2019

Landed in 84aa192...74f8196. Thanks for the reviews.

@cjihrig cjihrig deleted the rimraf branch November 27, 2019 23:28
addaleax pushed a commit that referenced this pull request Nov 30, 2019
This is part of reworking the rimraf retry logic.

Refs: #30580
PR-URL: #30644
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Nov 30, 2019
This commit makes retries an opt-in feature by defaulting
to no automatic retries. This will be particularly important
once synchronous operations can sleep between attempts.

Refs: #30580
PR-URL: #30644
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Nov 30, 2019
This commit removes the emfileWait option. EMFILE errors are
now handled the same as any other retriable error.

Refs: #30580
PR-URL: #30644
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Nov 30, 2019
This commit adds a retryDelay option to rimraf which configures
the amount of time between retry operations.

Refs: #30580
PR-URL: #30644
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Nov 30, 2019
Co-authored-by: Thang Tran <[email protected]>
Fixes: #30482
Refs: #30499
Refs: #30580
PR-URL: #30644
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Dec 3, 2019
BridgeAR added a commit that referenced this pull request Dec 3, 2019
Notable changes:

* fs:
  * Reworked experimental recursive `rmdir()`  (cjihrig)
    #30644
    * The `maxBusyTries` option is renamed to `maxRetries`, and its
      default is set to 0. The `emfileWait` option has been removed,
      and `EMFILE` errors use the same retry logic as other errors.
      The `retryDelay` option is now supported. `ENFILE` errors are
      now retried.
* http:
  * Make maximum header size configurable per-stream or per-server
    (Anna Henningsen) #30570
* http2:
  * Make maximum tolerated rejected streams configurable (Denys
    Otrishko) #30534
  * Allow to configure maximum tolerated invalid frames (Denys
    Otrishko) #30534
* wasi:
  * Introduce initial WASI support (cjihrig)
    #30258

PR-URL: #30774
BridgeAR added a commit that referenced this pull request Dec 3, 2019
Notable changes:

* fs:
  * Reworked experimental recursive `rmdir()`  (cjihrig)
    #30644
    * The `maxBusyTries` option is renamed to `maxRetries`, and its
      default is set to 0. The `emfileWait` option has been removed,
      and `EMFILE` errors use the same retry logic as other errors.
      The `retryDelay` option is now supported. `ENFILE` errors are
      now retried.
* http:
  * Make maximum header size configurable per-stream or per-server
    (Anna Henningsen) #30570
* http2:
  * Make maximum tolerated rejected streams configurable (Denys
    Otrishko) #30534
  * Allow to configure maximum tolerated invalid frames (Denys
    Otrishko) #30534
* wasi:
  * Introduce initial WASI support (cjihrig)
    #30258

PR-URL: #30774
targos pushed a commit that referenced this pull request Jan 13, 2020
This is part of reworking the rimraf retry logic.

Refs: #30580
PR-URL: #30644
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jan 13, 2020
This commit makes retries an opt-in feature by defaulting
to no automatic retries. This will be particularly important
once synchronous operations can sleep between attempts.

Refs: #30580
PR-URL: #30644
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jan 13, 2020
This commit removes the emfileWait option. EMFILE errors are
now handled the same as any other retriable error.

Refs: #30580
PR-URL: #30644
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jan 13, 2020
This commit adds a retryDelay option to rimraf which configures
the amount of time between retry operations.

Refs: #30580
PR-URL: #30644
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Jan 13, 2020
Co-authored-by: Thang Tran <[email protected]>
Fixes: #30482
Refs: #30499
Refs: #30580
PR-URL: #30644
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
This is part of reworking the rimraf retry logic.

Refs: #30580
PR-URL: #30644
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
This commit makes retries an opt-in feature by defaulting
to no automatic retries. This will be particularly important
once synchronous operations can sleep between attempts.

Refs: #30580
PR-URL: #30644
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
This commit removes the emfileWait option. EMFILE errors are
now handled the same as any other retriable error.

Refs: #30580
PR-URL: #30644
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
This commit adds a retryDelay option to rimraf which configures
the amount of time between retry operations.

Refs: #30580
PR-URL: #30644
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this pull request Feb 6, 2020
Co-authored-by: Thang Tran <[email protected]>
Fixes: #30482
Refs: #30499
Refs: #30580
PR-URL: #30644
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants