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: remove read lock from config get #5114

Merged
merged 1 commit into from Sep 21, 2023
Merged

fix: remove read lock from config get #5114

merged 1 commit into from Sep 21, 2023

Conversation

mkhq
Copy link
Contributor

@mkhq mkhq commented Sep 20, 2023

What this PR does / why we need it:

This removes the lock when reading from a global or local config file. The read lock can lead to dead-lock in situations where we trigger concurrent reads, e.g. in projects having multiple modules with linked sources. @nilium has provided a reproducible example here: https://github.com/nilium/garden-locker, see discord thread for more details: https://discord.com/channels/817392104711651328/1151269956240031826

Is it safe to remove the read lock?

  • Concurrent reads are safe since they don't modify the state of the config.

  • Writes need the lock since they perform a read + write of the config, which in the case of concurrent writes could result in a trace with R1, R2, W2, W1. Process 1 overwrites the values from process 2 leading to a potentially incorrect state.

  • Write operations are atomic, we either read the state before or after the write. A concurrent read should not be able to read a partial state of the config.

  • Config data is always validated against the schema and throws an error in case of failure.

Note of caution

  • Since this happened for concurrent reads, it will also happen for concurrent writes. In the example project provided by @nilium (https://github.com/nilium/garden-locker), there were in total 12 modules with remote sources (referenced by repositoryUrl).

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

@mkhq mkhq requested review from edvald and a team September 20, 2023 08:24
@nilium
Copy link

nilium commented Sep 20, 2023

Confirming this fixes the lock file error that prevented Garden 0.13 from working here. I can't comment on the safety of removing the read lock, so have to leave that to others, but it's nice to see 0.13 working here considering the other improvements that brings.

Copy link
Member

@stefreak stefreak left a comment

Choose a reason for hiding this comment

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

I agree with your conclusions about safety, if writeFileAtomic works as intended on all supported platforms.

@TimBeyer
Copy link
Contributor

Are file write operations atomic? The node docs say this:

It is unsafe to use fsPromises.writeFile() multiple times on the same file without waiting for the promise to be settled.
Similarly to fsPromises.readFile - fsPromises.writeFile is a convenience method that performs multiple write calls internally to write the buffer passed to it.

I assume that the chances of reading a partial file are small, but are they zero?

@TimBeyer
Copy link
Contributor

Ah ignore my comment, I didn't realize we're actually using writeFileAtomic which Steffen mentioned.
No concerns from my side then.

@mkhq mkhq added this pull request to the merge queue Sep 21, 2023
Merged via the queue into main with commit 2967dc5 Sep 21, 2023
40 checks passed
@mkhq mkhq deleted the fix-lock-file-read-lock branch September 21, 2023 21:25
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

Successfully merging this pull request may close these issues.

None yet

4 participants