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

fix: disable caching when babel could not read/write cache #10557

Merged
merged 7 commits into from Oct 17, 2019

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Oct 15, 2019

Q                       A
Fixed Issues? Fixes #6906, fixes #9649
Patch: Bug Fix? Yes
Tests Added + Pass? Added test on permission issues
Documentation PR Link
Any Dependency Changes?
License MIT

In this PR we catch the IO error and disable caching when permission issue arises, or it the cache path resides in a readonly filesystem.

The following existsSync usage

if (fs.existsSync(file)) {
  fs.readFileSync(file)
}

is also replaced as it introduces duplicate IO operations. In general fs.existsSync should only be used

if the file won’t be used directly, for example when its existence is a signal from another process.

In the future I will open a separate PR to replace all similar usage in our codebase.

@JLHwung JLHwung added the PR: Polish 💅 A type of pull request used for our changelog categories label Oct 15, 2019
@nicolo-ribaudo
Copy link
Member

Is it possible to add a test somehow?

);
cacheDisabled = true;
break;
case "EROFS":
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Disclaimer: I have not tested on a readonly filesystem, this error code is copied from man 2 open

[EROFS] The named file resides on a read-only file system, and the file is to be modified.

@JLHwung
Copy link
Contributor Author

JLHwung commented Oct 16, 2019

@nicolo-ribaudo Added test on permission issues. On the readonly file system error, I think it is hard to reproduce on unit testing.

load();

process.nextTick(() => {
process.nextTick(() => {
Copy link
Member

Choose a reason for hiding this comment

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

[Q]: Why isn't one nextTick enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[A]: I thought the execution order of next tick tasks is not defined. Apparently I am wrong after reading https://nodejs.org/docs/latest/api/process.html#process_process_nexttick_callback_args, thank you for reminding me!

@nicolo-ribaudo
Copy link
Member

@JLHwung The failing test on windows it a test of this PR.

@JLHwung
Copy link
Contributor Author

JLHwung commented Oct 17, 2019

@nicolo-ribaudo The failing test on Windows is due to the following caveats

on Windows only the write permission can be changed [by fs.chmod]

I guess that the read error could only occur when the file is owned by another user and have ACL settings excluding current user to read it. There is no isReadable mode on Windows.

As it is difficult to construct such example on Windows CI, I disable this test on Windows.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Polish 💅 A type of pull request used for our changelog categories
Projects
None yet
3 participants