Skip to content

[ProcessLauncher] Explicitly set diagnostic port configs #5503

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

Conversation

mdh1418
Copy link
Member

@mdh1418 mdh1418 commented Jun 16, 2025

Partly addresses #5501

Child processes started by diagnostic tooling set a DOTNET_DiagnosticPort environment variable, specifying only the path.
The runtime defaults to a connect type port in suspend mode, which isn't clear from the end user's viewpoint, as logging will only reveal the DiagnosticPort value. Instead, we can mitigate confusion by explicitly setting the config from the tooling side.

Before

The runtime has been configured to pause during startup and is awaiting a Diagnostics IPC ResumeStartup command from a Diagnostic Port.
DOTNET_DiagnosticPorts="dotnet-trace-48696-20250614_161420.socket"
DOTNET_DefaultDiagnosticPortSuspend=0

Now

The runtime has been configured to pause during startup and is awaiting a Diagnostics IPC ResumeStartup command from a Diagnostic Port.
DOTNET_DiagnosticPorts="dotnet-trace-29004-20250616_134924.socket,suspend,connect"
DOTNET_DefaultDiagnosticPortSuspend=0

Child processes started by diagnostic tooling set a DiagnosticPort
specifying only the path. The runtime defaults to a connect type
port in suspend mode, which isn't clear from the end user's viewpoint,
as logging will only reveal the DiagnosticPort value. Instead, we
can mitigate confusion by explicitly setting the config.
@mdh1418 mdh1418 requested a review from a team as a code owner June 16, 2025 17:50
@hoyosjs
Copy link
Member

hoyosjs commented Jun 16, 2025

This will not really solve the issue for the customer sadly - this is something that needs a little more work in trace to at least give better warnings to the tool. I'm ok with letting this go in - but we need to warn about child processes/document this.

Copy link
Member

@noahfalk noahfalk left a comment

Choose a reason for hiding this comment

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

The change looks fine, but we should call out that this is just one element of fixing the overall problem. We should not mark the issue fixed based on this PR alone.

Copy link
Member

@lateralusX lateralusX left a comment

Choose a reason for hiding this comment

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

The underlying problem is that the child processes inherit env variables by default, so if env variables are not going to be inherited by a child processes spawned by the process that got spawned by dotnet-trace (and have its DOTNET_DiagnosticPorts set), it needs to be cleared, but only for the env used when spawning the child process. The code spawning the child process needs to make sure this env is not inherited in its call to Process.Start.

@mdh1418
Copy link
Member Author

mdh1418 commented Jun 17, 2025

Thanks everyone for all the clarifying comments, modified to not close the tracking issue. Will follow Noah's outline and have this change just clarify that the env vars will suspend the runtime in the child process.

@mdh1418 mdh1418 merged commit 71de7f1 into dotnet:main Jun 17, 2025
19 checks passed
@mdh1418 mdh1418 deleted the explicit_diagnostics_ports_for_child_proc branch June 17, 2025 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants