-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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 feature switch for disabling startup hooks #44050
Conversation
Tagging subscribers to this area: @safern, @ViktorHofer |
Startup hook is one of many places that call |
if (!IsSupported) | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would structure this so that if someone specifies a startup hook (there's something actionable in STARTUP_HOOKS) and startup hook support was linked out, they get some sort of feedback about it (exception?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is better to ignore it silently (no exception).
Having ability to disable startup hooks per app is useful on its own, independent of linking. For example, STARTUP_HOOKS can be set globally for APM, but there is one utility where you do not want it to kick in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any chance we can add a test that has both a startup hook defined, and sets the app context switch to false
, and verifies that the startup hook is not executed?
@jkotas - Isn't it any dynamic load of an assembly? The linker needs to see every assembly that will be loaded in the app, or else it may trim code that the dynamically loaded assembly needed. |
Yes, it is any dynamic load. All dynamic assembly loads from disk file go through |
I think this is a good option, and I'd rather have one feature switch to disable all the places where a dynamic load can happen than one for each. Unless there's a scenario in which we'd like to use |
The Assembly.LoadFrom APIs and friends are already annotatated as linker unfriendly. Linker will warn whenever the codepath that calls into them is reachable. The objective of this pull request is to get rid of that warning - startup hook is compiled into an empty console app, so everyone who does trimming would see the warning. I'm not sure I like the idea of having a "feature switch" that makes LoadFrom and friends throw, with what I assume is no linker warnings - it's a pretty big hammer. Everyone who does any sort of linking is going to have to disable this (because they get warnings from the StartupHook use from the get-go). Once LoadFrom is disabled globally, people won't get a warning when they e.g. add a NuGet package that calls into LoadFrom in some obscure scenario. The end result is going to be a broken app. It feels like too big of a hammer. |
Ah, I have not realized that this change is required to fix IL warning that you get for LoadFromPath use inside startup hook today. It makes sense now. To answer my own questions:
Yes, we expect to have a feature switch for each use of LoadFromPath that is optional feature like startup hook. Each of these optional features will be disabled for trimmed apps by default.
Warnings that the linker issues for LoadFromAssemblyPath today cover this case already. Having a switch like this may be sometimes useful in addition to the warnings we get today, but it is not helping with what we are focused on now (drive IL linker warnings to zero). |
src/coreclr/src/System.Private.CoreLib/src/System/StartupHookProvider.cs
Outdated
Show resolved
Hide resolved
How do we make startup hook disabled by default for trimmed apps so that you get no warnings out of the box? |
Tagging subscribers to this area: @vitek-karas, @agocke |
We can use this feature switch in the SDK everytime |
Yeah, but that doesn't pass the feature switch that disables startup hooks. Startup hooks are enabled by default on .NET Core, unfortunately. So we need to make a call on whether we disable them by default when PublishTrimmed is specified, disable them by default in general (so that there's no secretly disabled functionality that one needs to dig out of docs when PublishTrimmed is specified), or whether people need to do two things to actually get usable trimming - specify PublishTrimmed and specify the feature switch. I wish when startup hooks got added, they were added as opt-in. :( |
Making startup hooks opt-in would not full-fill the requirements that they were added for originally. |
I think it is the right option. |
We've been taking a position that PublishTrimmed should never lead to observable changes in behavior. If a change in behavior is possible, there needs to be a warning. Having a warning for all apps would be annoying so maybe we don't have any other choice than to break that promise here, but I don't want to take it lightly.
Would that be a possible problem for trimmed apps? |
I don't think that's entirely true. We publish in 5.0 with observable changes in behaviour, one example is UseSystemResourceKeys but there are many more which by default change app behaviour.
If I understood this feature it requires also runtime.json settings. Could we add a warning when there are such settings and PublishTrimmed is used? |
Just to clear things a little:
|
As far as users are concerns this is on by default for example for Blazor workload when published with trimming. https://github.com/dotnet/aspnetcore/blob/219ecd6880120215137f4d6ef0dd2aa62c780af2/src/Components/WebAssembly/Sdk/src/targets/Microsoft.NET.Sdk.BlazorWebAssembly.Current.targets#L58
The doc talks about env variable to be set to make it work https://github.com/dotnet/runtime/blob/master/docs/design/features/host-startup-hook.md#proposed-behavior. Though not sure how accurate that doc is |
I consider Blazor "special" in this regard. For one thing, publishing Blazor also turns trimming on by default - another difference from non-Blazor apps (at least for now). The env. variable is the mechanism to actually use the startup hook feature, but the code is active all the time. It's similar to event pipe - the feature is there all the time, but does basically nothing, unless something external connects to it. Similarly startup hooks are there all the time, and are activated via env. variable. |
Right, but Blazor is a niche scenario where we established users can get by without the message. Blazor is incompatible with the regular .NET in many other ways. I would be more careful about making it the default elsewhere.
The env variable is specified at runtime, not at compile time. We don't have a good spot to emit a warning - that's why I was suggesting throwing an exception - it feels better to indicate that we're not going to do something the user wants us to do than to silently not do it. |
One of the deployment patterns for startup hooks is to have them set in the global environment. The first thing that startup hook does is a check whether it is meant to be running for the app it got loaded for. If not, it exits and does nothing. Throwing an exception would break this deployment pattern. Startup hooks are a niche scenario, even more niche than Blazor. It would be nice to avoid teaching everybody about their interactions with trimming by having good defaults. |
Regarding the default for trimmed apps - I would very much like to have this off by default for trimmed apps (along with couple other feature switches like COM for example). The problem is the mechanics. I strongly believe that feature switches should behave consistently regardless of trimming or even publishing. Meaning my app should run the same way when I do In order to achieve that we need to have some way for the developer to specify "I want this app to support trimming" (or something like that), which is not the same as "Trim this app now". Currently the I think we will need similar mechanism for single-file, AOT and so on. I view feature switches as a tool we can use for other scenarios, not just trimming. In fact the I'm trying to write all of this down in some kind of a design doc which will tackle the end-to-end experience, unfortunately I don't have it in any shape which would make sense to share right now. |
…rovider.cs Co-authored-by: Jan Kotas <[email protected]>
I agree with this when it comes to the inner working of app. I am less concerned when it comes to how the app interacts with the environment. For example, take self-contained publish vs. framework dependent publish. These two options are meant to be fully transparent. It holds for the most part, but breaks when it comes to interaction with the environment. For example, the app may check for specific files in the I see the startup hooks in this environmental category. Other features that fall into this category include managed profilers or "watch window" debugging experience. |
I'm not so sure about that. I think there are legit scenarios which will never care about trimming, mostly complex apps with a shared framework like VS. Why should we limit everyone to a common ground if the needs are different for large and small apps?
I don't think that hods anymore. It might be possible for some scenarios but as a general rule, I don't think we can say that. I think self-contained became a synonym for workload-specific output and not something that is a choice for any configuration.
We have that already but we don't use it that way but we should. I agree we should leverage
Agree but it's called feature-switch and it's about features, some are small whereas others are large. In this case, I think it's fine to have it (perhaps rename it to host startup hook) as it's a feature even though is not well documented and supported by CoreCLR only. |
I don't see the problem here to be honest. If a given app doesn't care about trimming that's fine. Some features will be "broken" when trimming (startup hooks as an example), those features will produce warnings. But if I can build my app without warnings its behavior should be consistent across all of the builds where I don't get warnings. I think @jkotas take on this is the right one - the behavior will not be identical, different build configurations are intentionally modifying some behavior of the app (debug enables better debugging experience, self-contained changes deployment requirements, ...). But features which are not intentionally affected by the change of configuration should NOT be affected. Crazy example: building the app as Debug should not run it with a different default culture when compared to Release. It's kind of hard to make a broad statement, since there will always be exceptions and gotchas, but I would much like to make a relatively simple statement about the consistency of behavior across different build/publish configurations. To me it's about expectations - clicking a single check-box in VS to enable single-file doesn't "feel" dangerous, so it should not break my app (or if it will, it should tell me -> warnings).
I think that some verticals will not support framework-dependent and that's OK, but those which will should behave consistently across the two configurations. To that end the verticals which don't support framework-dependent should warn if I try to force it - building Blazor client WASM app should fail/warn if I explicitly state --self-contained false. (It actually does fail today, but for different reasons, which is not ideal). |
src/coreclr/src/System.Private.CoreLib/src/System/StartupHookProvider.cs
Outdated
Show resolved
Hide resolved
[RequiresUnreferencedCode("Trimming may remove assemblies, types or members used by startup hooks. " + | ||
"These can be disabled using the StartupHookSupport feature switch")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@samsp-msft can you help us here with the message wording?
This is the message for a warning which will show up if an app allows startup hooks but asks for trimming. Since startup hooks by design load "random" code into the process, that code may have dependencies on things which were trimmed and thus it won't work.
The goal of the message should be:
- Describe what the problem is - short version "startup hooks are not compatible with trimming"
- At least hint at possible solution - in this case use the StartupHookSupport feature switch to disable startup hooks.
My attempt:
"This application enables startup hooks which might require assemblies, types or members removed during trimming. Startup hooks can be disabled with the StartupHookSupport feature switch."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential Incompatibility: The StartupHookSupport feature switch has been enabled for this app which is being trimmed. Startup hook code is not observable by the trimmer and so required assemblies, types and members may be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM pending the warning message change
Co-authored-by: Vitek Karas <[email protected]>
Looks good. Just so that you're aware. Once this is merged and the new runtime makes it to the sdk repo, the CI will fail on it in sdk, because of this test: https://github.com/dotnet/sdk/blob/0ae5136153da2d364288be1d9cdd4804d8279adf/src/Tests/Microsoft.NET.Publish.Tests/GivenThatWeWantToRunILLink.cs#L288 You will need to update the test in the 'update dependencies' PR to push it through. |
Fix #36526. This introduces a new feature switch which allows the
ILLinker
to trim the startup hooks logic.