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

Add 4 retries for fs.promises.rm #3351

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add 4 retries for fs.promises.rm #3351

wants to merge 1 commit into from

Conversation

devlikepro
Copy link
Contributor

@devlikepro devlikepro commented Oct 25, 2024 •
edited
Loading

PR Details

Description

Time to time it's possible to get some errors when logout - probably due to chrome process locking some files.

fs.promises.rm has maxRetries option for that https://nodejs.org/api/fs.html#fspromisesrmpath-options
We can set it to 4, giving us 4 retries with about 1s max delay (100+200+300+400=1,000ms)

Types of changes

  • Dependency change
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • I have updated the documentation accordingly (index.d.ts).
  • I have updated the usage example accordingly (example.js)

@anggerpr
Copy link

Wouldn't it be wiser if the maximum retry constant could be changed?

@devlikepro
Copy link
Contributor Author

@anggerpr good idea, will do 👍

@devlikepro devlikepro force-pushed the rm-retries branch 3 times, most recently from d7796b9 to ed9b8d2 Compare October 28, 2024 05:11
@devlikepro
Copy link
Contributor Author

done!

@devlikepro devlikepro mentioned this pull request Nov 5, 2024
Open
2 tasks
tuyuribr
tuyuribr approved these changes Nov 22, 2024
View reviewed changes
Copy link
Collaborator

@tuyuribr tuyuribr left a comment

Choose a reason for hiding this comment

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

LGTM

@alechkos
Copy link
Collaborator

@devlikepro please update your branch by main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tuyuribr tuyuribr tuyuribr approved these changes

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@devlikepro @anggerpr @alechkos @tuyuribr