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

write api file automically #7282

Merged
merged 3 commits into from May 10, 2020
Merged

Conversation

poonai
Copy link
Contributor

@poonai poonai commented May 6, 2020

Fix: #7227
Signed-off-by: Tiger rbalajis25@gmail.com

Signed-off-by: Tiger <rbalajis25@gmail.com>
Copy link
Member

@Stebalien Stebalien left a comment

Choose a reason for hiding this comment

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

A couple of nits but otherwise LGTM.

f, err := os.Create(filepath.Join(r.path, apiFile))
// Create a temp file to write the address, so that we don't leave empty file when the
// program crashes after creating the file.
f, err := os.Create(filepath.Join(r.path, apiFile+"-tmp"))
Copy link
Member

Choose a reason for hiding this comment

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

I'd name this something like:

Suggested change
f, err := os.Create(filepath.Join(r.path, apiFile+"-tmp"))
f, err := os.Create(filepath.Join(r.path, "." + apiFile+".tmp"))

}

// Atomically rename the temp file to the correct file name.
return os.Rename(filepath.Join(r.path, apiFile+"-tmp"), filepath.Join(r.path, apiFile))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return os.Rename(filepath.Join(r.path, apiFile+"-tmp"), filepath.Join(r.path, apiFile))
return os.Rename(filepath.Join(r.path, "." + apiFile+".tmp"), filepath.Join(r.path, apiFile))

@@ -359,14 +359,22 @@ func (r *FSRepo) Path() string {

// SetAPIAddr writes the API Addr to the /api file.
func (r *FSRepo) SetAPIAddr(addr ma.Multiaddr) error {
Copy link
Member

Choose a reason for hiding this comment

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

If we fail to rename the temporary file, let's remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to keep as it is. It feels like too much defensive programming.

When users run the program next time, newly created tmp will override the existing tmp file.

If you like to remove, I'll add code to remove the tmp file if rename fails

Copy link
Member

Choose a reason for hiding this comment

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

Well, it is defensive programming. While this is unlikely to fail on Linux, renaming can fail on Windows if some other user is accessing the API file at the same time. Yes, we'll end up deleting the temporary file later, but that isn't a reason to not at least try to clean it up early.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made changes. Thanks for the explaining the reason.

poonai added 2 commits May 7, 2020 23:34
Signed-off-by: Tiger <rbalajis25@gmail.com>
Signed-off-by: Tiger <rbalajis25@gmail.com>
@Stebalien Stebalien merged commit fa8c88b into ipfs:master May 10, 2020
@poonai poonai deleted the balaji/create_file_atomically branch May 11, 2020 15:36
@Stebalien Stebalien mentioned this pull request May 26, 2020
77 tasks
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.

Atomically create API file
2 participants