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/7.0] Disable EventSource support in NativeAOT by default #76052

Merged
merged 5 commits into from
Sep 28, 2022

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 23, 2022

Backport of #76000 to release/7.0

/cc @MichalStrehovsky

Customer Impact

DiagnosticSource is currently not AOT compatible. If a machine-wide DiagnosticSource-related event listener is enabled (such as PerfView, or possibly even a managed VS debugging session) it activates DiagnosticSource code paths within the executable and basically injects a runtime failure into NativeAOT processes due to the AOT-incompatibility of the code.

E.g. trying to do a HttpClient web request with PerfView collecting in the background causes a runtime exception to be thrown.

This uses the documented switch to disable EventSource support (unless the user specified a different value). Indirectly, it disables DiagnosticSource as well.

Testing

All NativeAOT testing.

Risk

This is a supported switch that we're just changing the default value for. The risk is low.

`DiagnosticSource` is currently not AOT compatible. If a machine-wide DiagnosticSource-related event listener is enabled (such as PerfView, or possibly even a managed VS debugging session) it activates `DiagnosticSource` code paths within the executable and basically injects a runtime failure into NativeAOT processes due to the AOT-incompatibility of the code.

E.g. trying to do a `HttpClient` web request with PerfView collecting in the background causes a runtime exception to be thrown.

This uses the documented switch to disable `EventSource` support (unless the user specified a different value). Indirectly, it disables `DiagnosticSource` as well.

As a side effect, disabling `EventSource` drops the size of a NativeAOT-compiled Hello World from 3.48 MB to 2.85 MB 🥳.
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

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.

approved. please get a code review and check the failing ci. we will take for consideration in ga.

@jkotas
Copy link
Member

jkotas commented Sep 23, 2022

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkotas
Copy link
Member

jkotas commented Sep 24, 2022

This needs test fixes from #76114

@MichalStrehovsky
Copy link
Member

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky
Copy link
Member

This needs test fixes from #76114

I cherry picked the necessary fixes in d97afaf. I assume those are de minimis test fixes that don't need extra reviewing.

(I don't understand how I missed those failures. I even wrote a regex to help me filter the known failing and unrelated tests in extra-platforms :/. There were many unrelated failures.)

@jkotas
Copy link
Member

jkotas commented Sep 26, 2022

I don't understand how I missed those failures. I even wrote a regex to help me filter the known failing and unrelated tests in extra-platforms :/. There were many unrelated failures.

When there are many unrelated failures, people always miss new ones even when super careful. It happened to me multiple times. If we end up in a state with many unrelated failures, we need to make fixing that a top priority.

@MichalStrehovsky
Copy link
Member

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@carlossanlop
Copy link
Member

@MichalStrehovsky don't forget to send the email to Tactics requesting approval, if you haven't done so.

@JulieLeeMSFT
Copy link
Member

@MichalStrehovsky please check the failed tests as well.

@MichalStrehovsky
Copy link
Member

The failures in Mono legs are unrelated.

@carlossanlop
Copy link
Member

carlossanlop commented Sep 28, 2022

@carlossanlop carlossanlop merged commit 4e78844 into release/7.0 Sep 28, 2022
@carlossanlop carlossanlop deleted the backport/pr-76000-to-release/7.0 branch September 28, 2022 21:27
@ghost ghost locked as resolved and limited conversation to collaborators Oct 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants