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 race condition in config handling (main) #2937

Merged
merged 4 commits into from
May 22, 2024

Conversation

f-blass
Copy link
Contributor

@f-blass f-blass commented May 21, 2024

Description of the Change

The CLI encounters race conditions when handling config files if multiple commands are executed in parallel.

Currently, the CLI writes a new config for every command executed due to the code in command_parser.go. However, it also deletes all temporary config files when reading the config, leading to race conditions between multiple processes. This PR introduces two changes to mitigate this problem:

  1. Ignore "File Not Found" errors when attempting to delete a file. This change addresses the issue where multiple processes list and delete temporary files simultaneously.
  2. Only delete temp config files older than 5 minutes. This change resolves the problem where one process creates a temporary config that it later wants to rename, while a second process deletes this temp config in the meantime. Orphan files should be rare, as they are normally pruned by the signal handler, as seen in
    func catchSignal(sig chan os.Signal, tempConfigFileName string) {
    <-sig
    _ = os.Remove(tempConfigFileName)
    os.Exit(2)

Why Is This PR Valuable?

Enables usage of the CF CLI in scripts which execute commands in parallel

Applicable Issues

fixes #2232

How Urgent Is The Change?

Medium

Other Relevant Parties

Anyone using the CLI in scripts with parallel execution. Multiple users have encountered this issue, as discussed in #2232.

Related PRs

v7
v8

Copy link
Member

@gururajsh gururajsh left a comment

Choose a reason for hiding this comment

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

LGTM

@gururajsh gururajsh merged commit 3eec49b into cloudfoundry:main May 22, 2024
12 checks passed
@f-blass f-blass deleted the fix-config-race-cond-main branch May 22, 2024 14:32
}
return err
}
// only delete old orphans which are not caught by the signal handler in WriteConfig
Copy link

@ryenus ryenus May 22, 2024

Choose a reason for hiding this comment

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

The condition check on the next line is for skipping the newer files. That's why it feels a little bit confusing to read this comment together with the next line.
It would be better to say something like: "skip newer files created within 5 minutes".
Nevertheless thank you @gururajsh so much for fixing this!

@f-blass
Copy link
Contributor Author

f-blass commented Jul 8, 2024

Hi @gururajsh
do you know when there will be a new release which includes this fix?

@gururajsh
Copy link
Member

Hi @f-blass
We are planning on doing a release this week which includes this fix. Will keep you posted.

@gururajsh
Copy link
Member

Hi @f-blass ,
New CLI version has been released with this fix

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.

Error writing config: rename ~/.cf/temp-config929711920 ~/.cf/config.json: no such file or directory
3 participants