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

Debugger IPC creation failure should not abort coreclr startup #90161

Merged
merged 3 commits into from
Aug 10, 2023

Conversation

tommcdon
Copy link
Member

@tommcdon tommcdon commented Aug 8, 2023

On Linux, the debugger creates 2 single-directional pipes in /tmp or $TMPDIR. Today if we fail to create these pipes, coreclr init will fail with Failed to create CoreCLR, HRESULT: 0x8007000E as described in #86259.

We found that there are two places where the runtime creates pipes/domain sockets for diagnostics:

  • Diagnostics IPC - By default the 'listen' port is created in /tmp or $TMPDIR if the env variable is set. It provides a transport for collecting memory dumps, starting an EventPipe tracing session, requesting a dump, ... It is considered non-fatal if it doesn't start.
  • Debugger Port - creates 2 pipes in /tmp or $TMPDIR if the variable is set in Debugger::Startup. This is considered fatal if we can't create the port. This is not only related to read only file systems or non-Linux file system mounts. For example, if $TMPDIR is set to a Windows mount in WSL2, the pipe will also fail to be created.

This PR changes Debugger Port creation failure from fatal to non-fatal.

@tommcdon tommcdon added this to the 8.0.0 milestone Aug 8, 2023
@tommcdon tommcdon requested a review from hoyosjs August 8, 2023 15:47
@ghost ghost assigned tommcdon Aug 8, 2023
@ghost
Copy link

ghost commented Aug 8, 2023

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

Issue Details

On Linux, the debugger creates 2 single-directional pipes in /tmp or $TMPDIR. Today if we fail to create these pipes, coreclr init will fail with Failed to create CoreCLR, HRESULT: 0x8007000E as described in #86259.

We found that there are two places where the runtime creates pipes/domain sockets for diagnostics:

  • Diagnostics IPC - By default the 'listen' port is created in /tmp or $TMPDIR if the env variable is set. It provides a transport for collecting memory dumps, starting an EventPipe tracing session, requesting a dump, ... It is considered non-fatal if it doesn't start.
  • Debugger Port - creates 2 pipes in /tmp or $TMPDIR if the variable is set in Debugger::Startup. This is considered fatal if we can't create the port. This is not only related to read only file systems or non-Linux file system mounts. For example, if $TMPDIR is set to a Windows mount in WSL2, the pipe will also fail to be created.

This PR changes Debugger Port creation failure from fatal to non-fatal.

Author: tommcdon
Assignees: -
Labels:

area-Diagnostics-coreclr

Milestone: 8.0.0

@tommcdon tommcdon requested a review from mikem8361 August 8, 2023 21:30
@@ -1939,7 +1939,8 @@ HRESULT Debugger::Startup(void)
if (FAILED(hr))
{
ShutdownTransport();
ThrowHR(hr);
STRESS_LOG0(LF_CORDB, LL_ERROR, "D::S: The debugger pipe failed to initialize in /tmp or $TMPDIR.\n");
return S_OK; // we do not want debugger IPC to block runtime initialization
Copy link
Member

Choose a reason for hiding this comment

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

Don't know if we should return s_ok in those cases. Given other paths do it, maybe ok for .net 8. We should have other values to tear down the RC thread.

Copy link
Member

Choose a reason for hiding this comment

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

We need to track properly cleaning up (returning S_FALSE here and if the env var is set so the caller does the cleanup) for .NET 9.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I reverted the change and will add the S_FALSE implementation in .NET 9

@mikem8361 mikem8361 self-requested a review August 9, 2023 21:00
@tommcdon tommcdon force-pushed the dev/tommcdon/graceDebuggerInitFail branch from 79cfeb5 to c9b4d5e Compare August 10, 2023 01:37
@tommcdon tommcdon merged commit 3b21b90 into dotnet:main Aug 10, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Sep 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants