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

WIP: Add mutex to temp_file_name() #1713

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

daniel-riehm
Copy link
Collaborator

I encountered test failures on Windows when running ctest -j12, and traced them back to temp_file_name() giving the same temporary filename to multiple tests. This PR makes the function more thread-safe by adding a mutex, which resolved the problem for me.

@kwcvrobot
Copy link
Collaborator

@kwcvrobot
Copy link
Collaborator

@kwcvrobot
Copy link
Collaborator

@kwcvrobot
Copy link
Collaborator

@mwoehlke-kitware
Copy link
Member

This is... very surprising. Unless I am missing something, why would a mutex help when the contention is across processes, which can't possibly contend on a mutex? More, why is _tempnam not returning a unique name?

@daniel-riehm daniel-riehm changed the title Add mutex to temp_file_name() WIP: Add mutex to temp_file_name() Dec 9, 2022
@daniel-riehm
Copy link
Collaborator Author

Upon further testing appears I was mistaken that this solves the problem. However, to answer @mwoehlke-kitware: the issue is that we are manually adding a suffix (e.g. .png) to the result of _tempnam. So if _tempnam returns abcdef, it is correct in that that file does not exist. But abcdef.png does exist, and that's what the function as a whole is returning once the suffix is added. Each test thread (or process?) independently gets abcdef from _tempnam, which is always the first filename this implementation of _tempnam tries, and that file never exists, because we only ever create abcdef.png. And then all the tests clobber over each other in abcdef.png. It seems adding .png, etc is necessary for indicating a particular image format to e.g. our OpenCV image writer, which takes a filename as argument. But it causes issues here.

This implementation of _tempnam (I'm using MSVC 2022) apparently just uses an increasing decimal number, instead of several pseudorandom characters like my Linux system. The temporary filename that several tests are clobbering over is test-2.tiff.

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.

3 participants