Fix race condition when creating unique filename

[luhenry]

Because the file is not opened with O_EXCL, there is no guarantee that multiple threads calling this function simultaneously across different processes will not generate the same filename. Instead make sure to use O_EXCL for the kernel to decide whether a filename already exists.

[google-cla[bot]]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[higher-performance]

Hi, I believe this limitation is by design. The documentation says:

// Returns a pathname for a file that does not currently exist.

and:

// There could be a race condition if two or more processes are calling this
// function at the same time -- they could both pick the same filename.

If nothing else, your change would make it so that the file does exist, which would (at least) break the contract specified in the documentation.

Have you checked all the call sites to see if this is OK? I believe we even have a test (DirectoryCreationTest.CreateDirectoriesAndUniqueFilename) that should break here:

TEST_F(DirectoryCreationTest, CreateDirectoriesAndUniqueFilename) {
  FilePath file_path2(FilePath::GenerateUniqueFileName(testdata_path_, FilePath("unique"), "txt"));
  ...
  EXPECT_FALSE(file_path2.FileOrDirectoryExists());  // file not there

Is this test not failing for you?

[luhenry]

Have you checked all the call sites to see if this is OK? I believe we even have a test (DirectoryCreationTest.CreateDirectoriesAndUniqueFilename) that should break here:

I've checked that all callers are fine with the file existing, and will simply open the path after the call to FilePath::GenerateUniqueFileName.

Would it be ok to keep the code generating the unique file name, updating the doc, and updating the test? It would also closely match what mktemp is doing.

Another alternative would be to match what mkstemp is doing by returning the open file descriptor directly rather than the file name, possibly in another function.

[higher-performance]

Have you checked all the call sites to see if this is OK? I believe we even have a test (DirectoryCreationTest.CreateDirectoriesAndUniqueFilename) that should break here:

I've checked that all callers are fine with the file existing, and will simply open the path after the call to FilePath::GenerateUniqueFileName.

Would it be ok to keep the code generating the unique file name, updating the doc, and updating the test? It would also closely match what mktemp is doing.

Another alternative would be to match what mkstemp is doing by returning the open file descriptor directly rather than the file name, possibly in another function.

Sorry, I'm still confused. I haven't had a chance to test your code, so I may be missing something, but how is this line not failing with your change?

EXPECT_FALSE(file_path2.FileOrDirectoryExists());  // file not there

If you create the file, shouldn't this line fail?

[luhenry]

Sorry, I'm still confused. I haven't had a chance to test your code, so I may be missing something, but how is this line not failing with your change?

The test indeed does fail, and I failed to test before submitting (my bad) as I was expecting some of the test to run on CI and validate the change. However, despite the test failing, I've been carrying this patch in multiple projects without issue.

That's why I am asking whether you'd be more comfortable with adding a new API similar to mkstemp rather than modifying the existing. We could then use that new API in places like XmlUnitTestResultPrinter or JsonUnitTestResultPrinter constructors. I'm happy to work on a quick proof of concept if that would help the discussion.

[higher-performance]

Ahh, okay I see. I think changing the existing API might be possible? Regardless -- it might be a bit before I get around to this, but I'll get back to you.

[higher-performance]

Hi, I took a deeper look at this. It turns out to be much less trivial than this PR suggests, since actually creating a file has additional failure modes (read-only FS, permission issues, etc.) which requires additional logic at all the call sites, as well as changing the APIs to return information about whether/why a file could(n't) be created. We don't believe this is a worthwhile change for us to make here, but thank you for raising the issue.