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

Add new Atomic File Operations rule #898

Merged
merged 1 commit into from Jul 26, 2022

Conversation

ydah
Copy link
Member

@ydah ydah commented Jun 27, 2022

Follow: rubocop/rubocop#10696

When doing file operations after confirming the existence check of a file, frequent parallel file operations may cause problems that are difficult to reproduce.
Therefore, it is preferable to use atomic file operations.

# bad - race condition with another process may result in an error in `mkdir`
unless Dir.exist?(path)
  FileUtils.mkdir(path)
end

# good - atomic and idempotent creation
FileUtils.mkdir_p(path)

# bad - race condition with another process may result in an error in `remove`
if File.exist?(path)
  FileUtils.remove(path)
end

# good - atomic and idempotent removal
FileUtils.rm_f(path)

@ydah ydah force-pushed the add_atomic_file_operation_rule branch from c9310ef to ae3426d Compare June 27, 2022 01:22
README.adoc Outdated Show resolved Hide resolved
README.adoc Outdated Show resolved Hide resolved
README.adoc Outdated Show resolved Hide resolved
@ydah ydah force-pushed the add_atomic_file_operation_rule branch from ae3426d to 380aacc Compare June 27, 2022 05:29
@ydah ydah requested a review from bbatsov June 27, 2022 05:39
README.adoc Outdated Show resolved Hide resolved
@ydah ydah requested a review from pirj June 27, 2022 07:54
README.adoc Outdated Show resolved Hide resolved
@ydah ydah force-pushed the add_atomic_file_operation_rule branch from 380aacc to c98d3cf Compare June 28, 2022 00:00
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Thank you!

README.adoc Outdated
Comment on lines 2195 to 2206
# bad
unless Dir.exist?(path)
FileUtils.mkdir(path)
end

if File.exist?(path)
FileUtils.remove(path)
end
Copy link
Member

Choose a reason for hiding this comment

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

It may be understood that those two snippets belong to a single example, while those are two separate ones and put corresponding good examples closer to bad ones.

WDYT of:

# bad - race condition with another process may result in an error in `mkdir`
unless Dir.exist?(path)
  FileUtils.mkdir(path)
end

# good - atomic and idempotent creation
FileUtils.mkdir_p(path)

# bad - race condition with another process may result in an error in `remove`
if File.exist?(path)
  FileUtils.remove(path)
end

# good - atomic and idempotent removal
FileUtils.rm_f(path)

The above addresses two problems:

  1. Confusion with what the example is
  2. Order of good and bad

Leaving the order (bad, bad, good, good vs bad, good, bad, good) up to you. I found one example of the second kind of ordering, but pretty sure one could find examples of bad, bad, good, good, too. So it's subjective and I leave it to your judgement how you want to do it.

Copy link
Member Author

@ydah ydah Jun 28, 2022

Choose a reason for hiding this comment

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

I was conforming to the nearby good/bad case style, but it may indeed be confusing in a case like this.
I thought the bad, good, bad, good order you gave as an example looked good and I'll rewrite it. Thank you so much.

Copy link
Contributor

Choose a reason for hiding this comment

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

I indeed like the explanation of what concurrency issue can arise, maybe even specify the errors that could happen: already exists for mkdir and does not exist for remove?

When doing file operations after confirming the existence check of a file,
frequent parallel file operations may cause problems that are difficult to reproduce.
Therefore, it is preferable to use atomic file operations.

```ruby
# bad - race condition with another process may result in an error in `mkdir`
unless Dir.exist?(path)
  FileUtils.mkdir(path)
end

# good - atomic and idempotent creation
FileUtils.mkdir_p(path)

# bad - race condition with another process may result in an error in `remove`
if File.exist?(path)
  FileUtils.remove(path)
end

# good - atomic and idempotent removal
FileUtils.rm_f(path)
```
@ydah ydah force-pushed the add_atomic_file_operation_rule branch from c98d3cf to 67cbf7f Compare June 28, 2022 08:24
@ydah ydah requested a review from pirj June 28, 2022 08:24
@ydah ydah changed the title Add new "Atomic File Operations" rule Add new _Atomic File Operations_ rule Jul 15, 2022
@ydah ydah changed the title Add new _Atomic File Operations_ rule Add new *Atomic File Operations* rule Jul 15, 2022
@ydah ydah changed the title Add new *Atomic File Operations* rule Add new Atomic File Operations rule Jul 15, 2022
@bbatsov bbatsov merged commit ea1f646 into rubocop:master Jul 26, 2022
@ydah ydah deleted the add_atomic_file_operation_rule branch July 26, 2022 20:33
koic added a commit to rubocop/rubocop that referenced this pull request Jul 27, 2022
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