Skip to content

Commit

Permalink
clean-up SpawnedProcess logging
Browse files Browse the repository at this point in the history
Summary:
# Context

On Windows, process creation was consistently failing with the following error:

```
class std::system_error: CreateProcess("C:\tools\eden\libexec\edenfsctl.exe" "--etc-eden-dir" "C:\ProgramData\facebook\eden" "redirect" "fixup" "--mount" "C:\open\fbsource") failed with err code: 87
```

Where error code 87 means "The parameters are incorrect"

It was unclear why this was happening. After hours of debugging, I've found that you can fix the problem by setting the stdin, stderr, and stdout of the subprocess to null. Something must be wrong with the way we handle inherriting default values for stdin/stderr, but the root cause is not entirely clear.

While I still don't fully understand the reason CreateProcess fails, it feels like this fix is safe enough to deploy for now.

# This diff

Now that we've diagnosed the issue, we can remove the extra logging that was added.

Reviewed By: kmancini

Differential Revision: D59783278

fbshipit-source-id: 8af3dd8ff5f9ad618e7f1508c74d978f478e1681
  • Loading branch information
MichaelCuevas authored and facebook-github-bot committed Jul 19, 2024
1 parent 173dea6 commit 525b210
Showing 1 changed file with 6 additions and 28 deletions.
34 changes: 6 additions & 28 deletions eden/common/utils/SpawnedProcess.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -593,30 +593,26 @@ SpawnedProcess::SpawnedProcess(

if (!startupInfo.StartupInfo.hStdInput) {
startupInfo.StartupInfo.hStdInput = GetStdHandle(STD_INPUT_HANDLE);
if (startupInfo.StartupInfo.hStdError == INVALID_HANDLE_VALUE) {
if (startupInfo.StartupInfo.hStdInput == INVALID_HANDLE_VALUE) {
auto err = makeWin32ErrorExplicit(
GetLastError(), "GetStdHandle(STD_INPUT_HANDLE) failed");
XLOGF(
ERR,
"Issue during SpawnedProcess Creation: {}",
"Issue during SpawnedProcess creation: {}",
folly::exceptionStr(err));
}
// Continue anyway; TODO(cuev): Determine if throwing immediately is the
// better option
handles.push_back(startupInfo.StartupInfo.hStdInput);
}
if (!startupInfo.StartupInfo.hStdOutput) {
startupInfo.StartupInfo.hStdOutput = GetStdHandle(STD_OUTPUT_HANDLE);
if (startupInfo.StartupInfo.hStdError == INVALID_HANDLE_VALUE) {
if (startupInfo.StartupInfo.hStdOutput == INVALID_HANDLE_VALUE) {
auto err = makeWin32ErrorExplicit(
GetLastError(), "GetStdHandle(STD_OUTPUT_HANDLE) failed");
XLOGF(
ERR,
"Issue during SpawnedProcess Creation: {}",
folly::exceptionStr(err));
}
// Continue anyway; TODO(cuev): Determine if throwing immediately is the
// better option
handles.push_back(startupInfo.StartupInfo.hStdOutput);
}
if (!startupInfo.StartupInfo.hStdError) {
Expand All @@ -629,8 +625,6 @@ SpawnedProcess::SpawnedProcess(
"Issue during SpawnedProcess Creation: {}",
folly::exceptionStr(err));
}
// Continue anyway; TODO(cuev): Determine if throwing immediately is the
// better option
handles.push_back(startupInfo.StartupInfo.hStdError);
}

Expand Down Expand Up @@ -702,33 +696,17 @@ SpawnedProcess::SpawnedProcess(
auto err = makeWin32ErrorExplicit(
errorCode,
fmt::format(
"CreateProcess({}) failed with cwd: {}, creation flags: {}, execPath: {}, env: {}",
"CreateProcess({}) failed with cwd: {}, creation flags: {}, execPath: {}",
wideToMultibyteString<std::string>(cmdLine),
options.cwd_.has_value() ? options.cwd_->viewWithoutUNC()
: "(no cwd provided)",
options.flags_.value_or(0),
options.execPath_.has_value() ? options.execPath_->view()
: "(no execPath provided)",
env));
XLOG(ERR) << folly::exceptionStr(err);
: "(no execPath provided)"));
XLOG(ERR, folly::exceptionStr(err));
throw err;
}

// Some times this code inexplicably fails with "The parameter is incorrect"
// error. To understand what successful CreateProcess() calls look like, we
// will temporarily add logging.
// TODO(cuev): Remove this logging once we get some data.
XLOGF(
DBG2,
"CreateProcess({}) succeeded with cwd: {}, creation flags: {}, execPath: {}, env: {}",
wideToMultibyteString<std::string>(cmdLine),
options.cwd_.has_value() ? options.cwd_->viewWithoutUNC()
: "(no cwd provided)",
options.flags_.value_or(0),
options.execPath_.has_value() ? options.execPath_->view()
: "(no execPath provided)",
env);

CloseHandle(procInfo.hThread);
proc_.reset(procInfo.hProcess);
#endif
Expand Down

0 comments on commit 525b210

Please sign in to comment.