-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
tests/tr1
: Fix sporadic failures caused by tmpnam
/_tempnam
#2210
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
CaseyCarter
approved these changes
Sep 18, 2021
CaseyCarter
approved these changes
Sep 20, 2021
AlexGuteniev
approved these changes
Sep 21, 2021
AnjuDel
reviewed
Sep 22, 2021
I'm mirroring this to an MSVC-internal PR. Please notify me if any further changes are pushed. |
AnjuDel
approved these changes
Sep 23, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While investigating failures that @mnatsuhara encountered in the internal test harness, @CaseyCarter encountered sporadic failures in the
tr1
legacy test suite:Click to expand test failure output from tests/tr1/tests/fstream1 and fstream2.
The problem appears to be that
tr1
usestmpnam
and_tempnam
to generate temporary file names. By default, these functions generate temporary file names in the user-global temp directory, and they're either short enough to be collision-prone, or sequential. 🙀 This is a problem when tests are run in parallel:Click to expand example output from tmpnam and _tempnam, plus the new function being added in this PR.
@cbezault explained that the GitHub test harness works around this problem by setting the
TMP
environment variable to a unique directory for each configuration of each test:STL/tests/utils/stl/test/format.py
Lines 149 to 152 in d6f9987
We didn't realize this was a problem for the internal test harness because it had an overall high level of flakiness (which we're trying to improve now), and because it reruns failed tests up to 10 times before logging the failure.
I believe we should permanently avoid this by generating truly unique names in the local directory. I'm adding a new header to generate names of the form
"temp_file_<64 hexits>.tmp"
and replacing all oftr1
's calls totmpnam
and_tempnam
. (I checkedstd
andtr1
for calls to the more moderntemp_directory_path()
, and none of those were problematic.)Note that because
tr1
is the legacy test suite, I am not attempting to refactor/cleanup nearby code, even though I see many things that could be improved/simplified. The only cleanups I'm performing are eliminatingNO_TMPNAM_TESTS
andtmpnam
macroization, to make it easier to see what needs to be changed.The new filenames that I'm generating are comfortably below the
100
andL_tmpnam
(260
) limits in the source code.I checked to see if any additional changes were needed, and I don't believe so.
tmpnam
either writes into a provided buffer, or returns a pointer to an internal buffer (thus, replacing it with a pointer into a namedstring
is always fine)._tempnam
allocates memory withmalloc
that should befree
d, and the tests were simply leaking this, so there are nofree
calls that need to be removed.I chose to use
std::
qualification intemp_file_name.h
instead of includingtr1
's central headers and using itsSTD
macro.