-
Notifications
You must be signed in to change notification settings - Fork 114
Reset the working directory of child processes we spawn on Windows. #1212
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
Reset the working directory of child processes we spawn on Windows. #1212
Conversation
@swift-ci test |
@swift-ci test Linux |
@swift-ci test |
1 similar comment
@swift-ci test |
I'm not planning to apply the same change to the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine to me. Agreed that making a similar change to the posix spawn path seems like a good idea. The temporary directory might also be a good CWD to use instead of the root directory if it's easier to get to / works better but the root directory seems fine too given that exit tests likely shouldn't rely on the CWD
@swift-ci test |
@swift-ci test |
@swift-ci test |
1 similar comment
@swift-ci test |
@swift-ci test Windows |
This PR modifies the Windows implementation of `spawnProcess()` so that it sets the working directory of the new process to "C:\" (or thereabouts). This prevents a race condition on Windows because that system won't let you delete a directory if it's the working directory of any process. See [The Old New Thing](https://devblogs.microsoft.com/oldnewthing/20101109-00/?p=12323) for a very on-the-nose blog post. Note that we do not specify the value of the working directory in an exit test body. A test should generally not rely on it anyway because it is global state and any thread could change its value at any time. I haven't written a unit test for this change because it's unclear what I could write that would be easily verifiable, and because I don't know what state I might perturb outside such a test by calling `SetCurrentDirectory()`. Resolves #1209. (This is a speculative fix.)
1dea725
to
7cb5040
Compare
@swift-ci test |
@swift-ci test Linux |
@swift-ci test macOS |
This PR modifies the Windows implementation of
spawnProcess()
so that it sets the working directory of the new process to "C:" (or thereabouts). This prevents a race condition on Windows because that system won't let you delete a directory if it's the working directory of any process. See The Old New Thing for a very on-the-nose blog post.Note that we do not specify the value of the working directory in an exit test body. A test should generally not rely on it anyway because it is global state and any thread could change its value at any time.
I haven't written a unit test for this change because it's unclear what I could write that would be easily verifiable, and because I don't know what state I might perturb outside such a test by calling
SetCurrentDirectory()
.Resolves #1209.
(This is a speculative fix.)
Checklist: