Skip to content

[3.1.x] Fix dotnet.exe process recovery after abnormal exit in ANCM #17103

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

Merged
merged 9 commits into from
Nov 22, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ class ConfigUtility
#define CS_ASPNETCORE_HANDLER_SETTINGS L"handlerSettings"
#define CS_ASPNETCORE_HANDLER_VERSION L"handlerVersion"
#define CS_ASPNETCORE_DEBUG_FILE L"debugFile"
#define CS_ASPNETCORE_ENABLE_OUT_OF_PROCESS_CONSOLE_REDIRECTION L"enableOutOfProcessConsoleRedirection"
#define CS_ASPNETCORE_DEBUG_LEVEL L"debugLevel"
#define CS_ASPNETCORE_HANDLER_SETTINGS_NAME L"name"
#define CS_ASPNETCORE_HANDLER_SETTINGS_VALUE L"value"
Expand All @@ -40,6 +41,13 @@ class ConfigUtility
return FindKeyValuePair(pElement, CS_ASPNETCORE_DEBUG_LEVEL, strDebugFile);
}

static
HRESULT
FindEnableOutOfProcessConsoleRedirection(IAppHostElement* pElement, STRU& strEnableOutOfProcessConsoleRedirection)
{
return FindKeyValuePair(pElement, CS_ASPNETCORE_ENABLE_OUT_OF_PROCESS_CONSOLE_REDIRECTION, strEnableOutOfProcessConsoleRedirection);
}

private:
static
HRESULT
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ PROCESS_MANAGER::GetProcess(
pConfig->QueryAnonymousAuthEnabled(),
pConfig->QueryEnvironmentVariables(),
pConfig->QueryStdoutLogEnabled(),
pConfig->QueryEnableOutOfProcessConsoleRedirection(),
fWebsocketSupported,
pConfig->QueryStdoutLogFile(),
pConfig->QueryApplicationPhysicalPath(), // physical path
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ SERVER_PROCESS::Initialize(
BOOL fAnonymousAuthEnabled,
std::map<std::wstring, std::wstring, ignore_case_comparer>& pEnvironmentVariables,
BOOL fStdoutLogEnabled,
BOOL fEnableOutOfProcessConsoleRedirection,
BOOL fWebSocketSupported,
STRU *pstruStdoutLogFile,
STRU *pszAppPhysicalPath,
Expand All @@ -43,6 +44,7 @@ SERVER_PROCESS::Initialize(
m_fWindowsAuthEnabled = fWindowsAuthEnabled;
m_fBasicAuthEnabled = fBasicAuthEnabled;
m_fAnonymousAuthEnabled = fAnonymousAuthEnabled;
m_fEnableOutOfProcessConsoleRedirection = fEnableOutOfProcessConsoleRedirection;
m_pProcessManager->ReferenceProcessManager();
m_fDebuggerAttached = FALSE;

Expand Down Expand Up @@ -1030,6 +1032,15 @@ SERVER_PROCESS::SetupStdHandles(
saAttr.bInheritHandle = TRUE;
saAttr.lpSecurityDescriptor = NULL;

if (!m_fEnableOutOfProcessConsoleRedirection)
{
pStartupInfo->dwFlags = STARTF_USESTDHANDLES;
pStartupInfo->hStdInput = INVALID_HANDLE_VALUE;
pStartupInfo->hStdError = INVALID_HANDLE_VALUE;
pStartupInfo->hStdOutput = INVALID_HANDLE_VALUE;
return hr;
}

if (!m_fStdoutLogEnabled)
{
CreatePipe(&m_hStdoutHandle, &m_hStdErrWritePipe, &saAttr, 0 /*nSize*/);
Expand Down Expand Up @@ -1770,6 +1781,8 @@ SERVER_PROCESS::SERVER_PROCESS() :
m_dwListeningProcessId(0),
m_hListeningProcessHandle(NULL),
m_hShutdownHandle(NULL),
m_hStdErrWritePipe(NULL),
m_hReadThread(nullptr),
m_randomGenerator(std::random_device()())
{
//InterlockedIncrement(&g_dwActiveServerProcesses);
Expand Down Expand Up @@ -1866,13 +1879,15 @@ SERVER_PROCESS::~SERVER_PROCESS()
m_pProcessManager = NULL;
}

if (m_hStdoutHandle != NULL)
if (m_hStdErrWritePipe != NULL)
{
if (m_hStdoutHandle != INVALID_HANDLE_VALUE)
if (m_hStdErrWritePipe != INVALID_HANDLE_VALUE)
{
CloseHandle(m_hStdoutHandle);
FlushFileBuffers(m_hStdErrWritePipe);
CloseHandle(m_hStdErrWritePipe);
}
m_hStdoutHandle = NULL;

m_hStdErrWritePipe = NULL;
}

// Forces ReadFile to cancel, causing the read loop to complete.
Expand Down Expand Up @@ -1907,6 +1922,15 @@ SERVER_PROCESS::~SERVER_PROCESS()
m_hReadThread = nullptr;
}

if (m_hStdoutHandle != NULL)
{
if (m_hStdoutHandle != INVALID_HANDLE_VALUE)
{
CloseHandle(m_hStdoutHandle);
}
m_hStdoutHandle = NULL;
}

if (m_fStdoutLogEnabled)
{
m_Timer.CancelTimer();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ class SERVER_PROCESS
_In_ BOOL fAnonymousAuthEnabled,
_In_ std::map<std::wstring, std::wstring, ignore_case_comparer>& pEnvironmentVariables,
_In_ BOOL fStdoutLogEnabled,
_In_ BOOL fDisableRedirection,
_In_ BOOL fWebSocketSupported,
_In_ STRU *pstruStdoutLogFile,
_In_ STRU *pszAppPhysicalPath,
Expand Down Expand Up @@ -253,6 +254,7 @@ class SERVER_PROCESS
BOOL m_fBasicAuthEnabled;
BOOL m_fAnonymousAuthEnabled;
BOOL m_fDebuggerAttached;
BOOL m_fEnableOutOfProcessConsoleRedirection;

STTIMER m_Timer;
SOCKET m_socket;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,12 @@ REQUESTHANDLER_CONFIG::Populate(
goto Finished;
}

hr = ConfigUtility::FindEnableOutOfProcessConsoleRedirection(pAspNetCoreElement, m_fEnableOutOfProcessConsoleRedirection);
if (FAILED(hr))
{
goto Finished;
}

Finished:

if (pAspNetCoreElement != NULL)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,12 @@ class REQUESTHANDLER_CONFIG
return &m_struConfigPath;
}

BOOL
QueryEnableOutOfProcessConsoleRedirection()
{
return !m_fEnableOutOfProcessConsoleRedirection.Equals(L"false", 1);
}

protected:

//
Expand Down Expand Up @@ -255,6 +261,7 @@ class REQUESTHANDLER_CONFIG
BOOL m_fWindowsAuthEnabled;
BOOL m_fBasicAuthEnabled;
BOOL m_fAnonymousAuthEnabled;
STRU m_fEnableOutOfProcessConsoleRedirection;
APP_HOSTING_MODEL m_hostingModel;
std::map<std::wstring, std::wstring, ignore_case_comparer> m_pEnvironmentVariables;
STRU m_struHostFxrLocation;
Expand Down
16 changes: 16 additions & 0 deletions src/Servers/IIS/IIS/test/Common.FunctionalTests/LogFileTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,22 @@ public async Task CaptureLogsForOutOfProcessWhenProcessFailsToStart()
EventLogHelpers.VerifyEventLogEvent(deploymentResult, EventLogHelpers.OutOfProcessFailedToStart(deploymentResult, "Wow!"), Logger);
}

[ConditionalFact]
[RequiresNewShim]
public async Task DisableRedirectionNoLogs()
{
var deploymentParameters = Fixture.GetBaseDeploymentParameters(HostingModel.OutOfProcess);
deploymentParameters.HandlerSettings["enableOutOfProcessConsoleRedirection"] = "false";
deploymentParameters.TransformArguments((a, _) => $"{a} ConsoleWriteSingle");
var deploymentResult = await DeployAsync(deploymentParameters);

var response = await deploymentResult.HttpClient.GetAsync("Test");

StopServer();

EventLogHelpers.VerifyEventLogEvent(deploymentResult, EventLogHelpers.OutOfProcessFailedToStart(deploymentResult, ""), Logger);
}

[ConditionalFact]
public async Task CaptureLogsForOutOfProcessWhenProcessFailsToStart30KbMax()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,55 @@ public async Task EnvVarInWebConfig_Invalid(TestVariant variant, string port)
Assert.Equal(HttpStatusCode.BadGateway, response.StatusCode);
}

[ConditionalTheory]
[MemberData(nameof(TestVariants))]
[RequiresNewShim]
public async Task ShutdownMultipleTimesWorks(TestVariant variant)
{
// Must publish to set env vars in web.config
var deploymentParameters = Fixture.GetBaseDeploymentParameters(variant);

var deploymentResult = await DeployAsync(deploymentParameters);

// Shutdown once
var response = await deploymentResult.HttpClient.GetAsync("/Shutdown");

// Wait for server to start again.
int i;
for (i = 0; i < 10; i++)
{
// ANCM should eventually recover from being shutdown multiple times.
response = await deploymentResult.HttpClient.GetAsync("/HelloWorld");
if (response.IsSuccessStatusCode)
{
break;
}
}

if (i == 10)
{
// Didn't restart after 10 retries
Assert.False(true);
}

// Shutdown again
response = await deploymentResult.HttpClient.GetAsync("/Shutdown");

// return if server starts again.
for (i = 0; i < 10; i++)
{
// ANCM should eventually recover from being shutdown multiple times.
response = await deploymentResult.HttpClient.GetAsync("/HelloWorld");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still not a fan 😆

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the problem here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test is a bit... ugly.

if (response.IsSuccessStatusCode)
{
return;
}
}

// Test failure if this happens.
Assert.False(true);
}

private static int GetUnusedRandomPort()
{
// Large number of retries to prevent test failures due to port collisions, but not infinite
Expand Down