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

Race condition in mtime-precision/probe #103

Open
julien-f opened this issue May 8, 2021 · 4 comments
Open

Race condition in mtime-precision/probe #103

julien-f opened this issue May 8, 2021 · 4 comments

Comments

@julien-f
Copy link

julien-f commented May 8, 2021

Two parallel calls, cachedPrecision is not defined in both, both will call utimes then stat, and Object.defineProperty will be called twice.

This does not throw TypeError: Cannot redefine property because the new value to be defined is the existing value.

@julien-f
Copy link
Author

julien-f commented May 8, 2021

The problem can appear though if trying to lock files on different fs with different precisions.

Maybe that should be a separate issue though.

@satazor
Copy link
Contributor

satazor commented May 8, 2021

Hello @julien-f, if I understood correctly, the precision can vary when locking different paths that are associated with different file systems, is that correct?

In those situations, Object.defineProperty can throw or maybe a previous precision will be used which is wrong.

I’m not sure if we can do much here. There are a few options:

  • Issue a big warning in the README to provide a different options.fs (like cloning it or using Object.create) if you are using different filesystems.
  • Create a new options where you provide paths to different filesystems. We will use this to calculate the precision for each and save it in an object where keys are the paths and values are the precision.
  • Completely disable caching but things will slow down.

@julien-f
Copy link
Author

julien-f commented May 9, 2021

the precision can vary when locking different paths that are associated with different file systems, is that correct?

Yep.

IMHO, those duplicate calls to Object.defineProperty are incorrect but it may work well enough as it is to avoid the complexity of deduplicating probing.

I see a possible alternative to your ideas: the precision could be cached per device (fs.Stats#dev).

@satazor
Copy link
Contributor

satazor commented May 9, 2021

Didn’t know about the device. It seems we can indeed use that. Are you open to do a PR?

julien-f added a commit to julien-f/node-proper-lockfile that referenced this issue May 10, 2021
julien-f added a commit to julien-f/node-proper-lockfile that referenced this issue May 10, 2021
julien-f added a commit to julien-f/node-proper-lockfile that referenced this issue May 10, 2021
julien-f added a commit to julien-f/node-proper-lockfile that referenced this issue May 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants