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

[release/6.0] Don't cache commandline in coreclr diagnostics server #63356

Merged
merged 5 commits into from
May 3, 2022

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Jan 4, 2022

Backport of #60275 to release/6.0

/cc @josalem

Customer Impact

When we switched to the C implementation of the diagnostics server and eventpipe in .net6, we added some caching around some values that we return in IPC commands. Some of these values, however, actually changed depending on when the IPC command was sent. Specifically, the entry point assembly and commandline reported by the runtime are different at the suspension point than after execution has started for non-Windows platforms. This is due to a limitation of hosting infrastructure and the PAL on non-Windows platforms. If these values are requested while suspended, then the mock values will be cached for the rest of execution which is a regression from .net5 behavior.

Behavior before this change (non-Windows only): requesting the commandline while the runtime is suspended or before the PAL/hosting layer calculate the commandline will cache a mock value of dotnet instead of the actual commandline for the rest of execution.

Behavior after this change (non-Windows only): requesting the commandline before it is calculated will still return the mock value of dotnet, but subsequent requests after it is calculated will return the correct value.

fixes #64270

Testing

There are existing tests for this feature that are still passing and a test has been added to validate the non-caching behavior in Unix platforms.

Risk

This is reverting to prior behavior and reducing code complexity. As a result, there is minimal risk with this change.

…point (#63382)

This only applies to CoreCLR Unix processes.
@ghost ghost closed this Mar 5, 2022
@ghost
Copy link

ghost commented Mar 5, 2022

Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it.

@jkotas jkotas deleted the backport/pr-60275-to-release/6.0 branch March 5, 2022 23:51
@josalem josalem restored the backport/pr-60275-to-release/6.0 branch March 14, 2022 16:57
@ericstj ericstj reopened this Mar 14, 2022
@josalem
Copy link
Contributor

josalem commented Mar 14, 2022

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@josalem josalem marked this pull request as ready for review March 14, 2022 18:12
@josalem
Copy link
Contributor

josalem commented Mar 17, 2022

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

Approve. We will take for consideration in 6.0.x. Please ensure to get a code review and resolve the pr failures.

@josalem
Copy link
Contributor

josalem commented Mar 21, 2022

I had to run the android tests several times before they passed. The failures were all related to helix timeouts. I didn't see anything in the logs indicating anything related to this change.

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.

LGTM

src/coreclr/vm/eventing/eventpipe/ep-rt-coreclr.h Outdated Show resolved Hide resolved
src/coreclr/vm/eventing/eventpipe/ep-rt-coreclr.h Outdated Show resolved Hide resolved
@josalem
Copy link
Contributor

josalem commented Apr 1, 2022

Incoming change with an InterlockedCompareExchange to guarantee we only leak one string in the rare race. Testing locally first.

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.

LGTM

@josalem
Copy link
Contributor

josalem commented Apr 4, 2022

The CI failures are due to the Win-x64 Release SingleFile leg trying to use a Helix queue that doesn't exist: https://dev.azure.com/dnceng/public/_build/results?buildId=1695726&view=logs&j=f30da5b4-0d0d-502a-ba45-e575885ba318&t=456725c9-9234-575b-34cc-2844ffe3f24e&l=91

Specifically, Windows.10.Amd64.Server19H1.ES.Open.

CI should pass if/when #67554 #67054 is merged.

@carlossanlop
Copy link
Member

This is missing the servicing-consider label so it didn't go through Tactics.

@carlossanlop carlossanlop added the Servicing-consider Issue for next servicing release review label Apr 13, 2022
@josalem
Copy link
Contributor

josalem commented Apr 13, 2022

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 14, 2022
@leecow leecow modified the milestones: 6.0.x, 6.0.6 Apr 14, 2022
@josalem
Copy link
Contributor

josalem commented Apr 18, 2022

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@josalem
Copy link
Contributor

josalem commented Apr 20, 2022

I believe the remaining CI failures are all fixed infra issues that the branch isn't reflecting. I can't seem to force push to this branch to rebase it on top of release/6.0 since the backport bot created the branch. Is closing and reopening the PR (something I don't believe I have permissions to do either) the only other way to rebase it? CC @hoyosjs

@hoyosjs
Copy link
Member

hoyosjs commented Apr 20, 2022

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@carlossanlop carlossanlop merged commit dfb2789 into release/6.0 May 3, 2022
@carlossanlop carlossanlop deleted the backport/pr-60275-to-release/6.0 branch May 3, 2022 23:25
josalem pushed a commit to josalem/runtime that referenced this pull request May 4, 2022
josalem pushed a commit that referenced this pull request May 6, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Tracing-coreclr Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants