Skip to content

Conversation

@diego-plan9
Copy link
Contributor

Fixes #37558

Use a temporary file instead of /dev/null in ReducerTest, to prevent the chance of unintended deletion when running as root. It seemed that there were no strong side-effects (running time?) by fixing it at the test level, compared to other solutions that involved modifying the behaviour of FileStore (for example, adding an optional flag to avoid auto-deleting the file upon destruction).

Please note this is my first contribution - I have done my best to read the contributing guide and checked for duplicate PRs with no luck, but apologies in advance for any oversights and lack of familiarity with the procedures.

Use a temporary file instead of "/dev/null" in `ReducerTest`, to
prevent the chance of unintended deletion when running as root.
@dr-ci
Copy link

dr-ci bot commented May 26, 2020

💊 CI failures summary and remediations

As of commit e09fe4a (more details on the Dr. CI page):


  • 1/1 failures possibly* introduced in this PR
    • 1/1 non-CircleCI failure(s)

Extra GitHub checks: 1 failed


This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group.

See how this bot performed.

This comment has been revised 1 time.

Copy link
Contributor

@mrshenli mrshenli left a comment

Choose a reason for hiding this comment

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

Thanks for fixing! This LGTM. FileStore's dtor would delete the temporary file:

FileStore::~FileStore() {
// cleanup key will be different from all rest keys since all rest keys will
// have a regular prefix.
auto numFinishedWorker = addHelper(cleanupKey_, 1);
// The last worker cleans up the file
if (numFinishedWorker == numWorkers_) {
// Best effort removal without checking the return
std::remove(path_.c_str());
}
}

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@mrshenli has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@mrshenli merged this pull request in 4d1df74.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Running python tests as root breaks the /dev/null file

5 participants