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

Add new DiagnosticClient commands for IPC features #2268

Merged
merged 9 commits into from
Jun 28, 2021

Conversation

davmason
Copy link
Member

Client commands for dotnet/runtime#52175 and dotnet/runtime#52567

@davmason davmason requested review from josalem, sywhang and a team May 11, 2021 08:28
@davmason davmason self-assigned this May 11, 2021
Copy link
Contributor

@sywhang sywhang left a comment

Choose a reason for hiding this comment

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

LGTM - would be nice if you can also include a docs change in documentation/diagnostics-client-library-instructions.md since there are some public API additions. The test failures also seem to be coming from runtime that doesn't support the command so... may need to target the tests to versions that support this command after the runtime change gets merged

@davmason davmason force-pushed the client_updates branch 2 times, most recently from 022bc7a to f45da35 Compare June 11, 2021 19:54
@davmason
Copy link
Member Author

@mikem8361 @hoyosjs I see failures like this:

System.IO.FileNotFoundException : Dump file does not exist: /__w/1/s/artifacts/tmp/Release/dumps/ProjectK/3.1.15/netcoreapp3.1/SOS.DotnetDumpCommands.Heap.dcd.dmp

I don't see how my changes could cause this. Is this a known issue?

@mikem8361
Copy link
Member

The M.D.NETCore.Client's DiagnosticsClient.WriteDump API is failing before the message gets sent to the runtime and it looks like you made a lot of changes to that assembly that affected the WriteDump API.

@davmason
Copy link
Member Author

Ah, I see. Thanks Mike. I will take a look at that

@davmason
Copy link
Member Author

@josalem @hoyosjs can one of you review this? I found the failures - I was encoding a bool wrong in the WriteDump command.

Copy link
Contributor

@josalem josalem left a comment

Choose a reason for hiding this comment

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

A couple questions, but otherwise this LGTM

@davmason davmason merged commit dfb97e8 into dotnet:main Jun 28, 2021

This sample shows how to request that the runtime use an ICorProfiler as the startup profiler (not as an attaching profiler). It is only valid to issue this command while the runtime is paused in "reverse server" mode.
Copy link
Member

Choose a reason for hiding this comment

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

I think the reversed server is orthogonal, the runtime just needs to be paused at startup? That startup pause can be accomplished regardless whether the port is listening or connecting:

export DOTNET_DiagnosticPorts=foo.sock,connect,suspend
OR
export DOTNET_DiagnosticPorts=foo.sock,listen,suspend

@github-actions github-actions bot locked and limited conversation to collaborators Jan 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants