Allow runtimeconfig StartupHooks and Environment StartupHooks to both be present

[tomdegoede]

When building/running an app via our Bazel build implementation we rely on a startup hook for assembly loading. We specify this in the runtimeconfig.json as a configProperty STARTUP_HOOKS.

When another startup hook is bootstrapped via the environment variable DOTNET_STARTUP_HOOKS this results in the following error:

Duplicate runtime property found: STARTUP_HOOKS
It is invalid to specify values for properties populated by the hosting layer in the the application's .runtimeconfig.json

In our case this environment variable is added by the IDE to handle net6.0 HotReloading.

Technically it is allowed to specify multiple startup hooks with a delimiter. This pull request resolves above error by merging startup hooks when they are specified in both locations.

Since I rarely visit C++ a detailed review is very welcome. I could not find any existing tests for this code path to extend/replicate but have tested that it resolves my issue. I have also taken the opportunity to fix some log_duplicate_property_errors that mistakenly list the StartUpHooks property key.

[ghost]

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

---

When building/running an app via our Bazel build implementation we rely on a startup hook for assembly loading. We specify this in the runtimeconfig.json as a configProperty STARTUP_HOOKS.

When another startup hook is bootstrapped via the environment variable DOTNET_STARTUP_HOOKS this results in the following error:

Duplicate runtime property found: STARTUP_HOOKS
It is invalid to specify values for properties populated by the hosting layer in the the application's .runtimeconfig.json

In our case this environment variable is added by the IDE to handle net6.0 HotReloading.

Technically it is allowed to specify multiple startup hooks with a delimiter. This pull request resolves above error by merging startup hooks when they are specified in both locations.

Since I rarely visit C++ a detailed review is very welcome. I could not find any existing tests for this code path to extend/replicate but have tested that it resolves my issue. I have also taken the opportunity to fix some log_duplicate_property_errors that mistakenly list the StartUpHooks property key.

Author:	tomdegoede
Assignees:	-
Labels:	area-Host
Milestone:	-

[dnfadmin]

All CLA requirements met.

[vitek-karas]

Looks good on the product side. We should add a test. The tests for startup hooks are here: https://github.com/dotnet/runtime/blob/main/src/installer/tests/HostActivation.Tests/StartupHooks.cs

(The host tests are a bit tricky to run - please take a look here: https://github.com/dotnet/runtime/blob/main/docs/workflow/testing/host/testing.md)

Thanks a lot for this!

[vitek-karas]

Question unrelated to the PR:
What is the scenario where you want to bake the startup hook into the .runtimeconfig.json?

[tomdegoede]

Question unrelated to the PR: What is the scenario where you want to bake the startup hook into the .runtimeconfig.json?

Our build tool (Bazel) doesn't copy dependencies to the output directory but instead lists their paths in a App.runfiles_manifest. To properly resolve the dll's at runtime we add a startup hook that registers Resolve handlers on AssemblyLoadContext.Default. In many scenarios we could register this via the environment variable, but in some cases (such as running tests from the IDE) this would be quite a hassle. The App.runtimeconfig.json of the target binary is always considered and thus fixes assembly loading in all cases. It also allows us to run bin/App.exe without requiring any additional environment variables, thus not requiring a wrapper or .bat file.

I'll look into the feedback. Thanks for the quick reply!

[tomdegoede]

Looks good on the product side. We should add a test. The tests for startup hooks are here: https://github.com/dotnet/runtime/blob/main/src/installer/tests/HostActivation.Tests/StartupHooks.cs

(The host tests are a bit tricky to run - please take a look here: https://github.com/dotnet/runtime/blob/main/docs/workflow/testing/host/testing.md)

Thanks a lot for this!

@vitek-karas I have added tests and reused the predefined PATH_SEPARATOR.

[vitek-karas]

Looks great - thanks a lot!

[vitek-karas]

Hmm - thinking about the order. I wonder if it would be better to run the env. variable hooks first.

In other places (for example roll forward) the env. variable takes precedence over runtime config.

Typically the order of startup hooks should not matter as they should not make assumptions about other possible startup hooks. But in reality that's not always the case, as typically the thing starting the process also "owns" the startup hook and sets up the env. variable accordingly.

I lean toward having the startup hooks from the env. variable run BEFORE those from the runtime config.

@sbomer, what do you think?

[tomdegoede]

Hmm - thinking about the order. I wonder if it would be better to run the env. variable hooks first.

In other places (for example roll forward) the env. variable takes precedence over runtime config.

Typically the order of startup hooks should not matter as they should not make assumptions about other possible startup hooks. But in reality that's not always the case, as typically the thing starting the process also "owns" the startup hook and sets up the env. variable accordingly.

I lean toward having the startup hooks from the env. variable run BEFORE those from the runtime config.

@sbomer, what do you think?

I think the env hook is already executed first since I append the existing config_startup_hooks to the env string_t. This was however not an explicit decision, nor do the tests currently validate it.

[sbomer]

Agreed, the env var should win, since the original design was addressing situations in which modifying deployed bits in the runtimeconfig was not an option.

[vitek-karas]

So we're in agreement, env. variable startup hooks should run before the runtime config ones.
@sbomer I just want to make sure we are in agreement:

• Runtime config registers a startup hook "RuntimeConfigHook"
• Env. variable is set to a startup hook "VariableHook"

Running the app will run both startup hooks, first the "VariableHook" and then the "RuntimeConfigHook" (and then the Main).

The code in this PR already works this way. Testing it will be a bit tricky, we don't have test helpers to test for order of messages in the output. The simplest way would be to grab the entire test output and implement the validation manually (search for first, and then search again from that point for the second).

I think we should try to add a test for this, it's an important detail and it's easy to break in the future if the code changes. Please add a comment into the code that the order is intentional.

[tomdegoede]

@vitek-karas I have updated the testcase to consider stdOut order and added a comment in the code path. Please let me know if there is anything else to improve! Thanks :)

[vitek-karas]

@sbomer, can you please check the code as well (just to make sure, since this is both fix and tiny design change in one 😉)

[sbomer]

LGTM, thank you!

[tomdegoede]

@vitek-karas small beginners question: will this end up in an upcoming patch version for net6 or only in the next net7? Ideally our issue is resolved in net6.

[vitek-karas]

Currently the main is for .NET 7 - so that will happen automatically.
.NET 6 would be harder - we would need to come up with a good reason to do so and all of the risk versus reward discussion.

On its own I like this - specifically because of hot-reload using startup hooks. That said, this is the first time I've heard anybody specifying startup hooks in the runtimeconfig. I'll ask others how they would feel about this.. currently for me, it probably doesn't feel important enough to include in servicing. But there might be different expectations for some of the future servicing releases of .NET 6, so maybe that will change.

[agocke]

This looks like a good fix to me. Low risk, addresses an oversight in startup hooks specified through two different mechanisms.

[agocke]

/backport to release/6.0

[github-actions]

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1482464925

[github-actions]

@agocke an error occurred while backporting to release/6.0, please check the run log for details!

Error: @agocke is not a repo collaborator, backporting is not allowed.

[agocke]

uhhh ?? @terrajobst assistance with the above?

[tomdegoede]

@agocke @terrajobst would be great if backporting is accepted! Thanks!

[terrajobst]

@agocke is not a repo collaborator, backporting is not allowed.

Your user is an org member and has push permissions. I've pinged @mmitche with details.

[terrajobst]

@tomdegoede

@agocke @terrajobst would be great if backporting is accepted! Thanks!

Yeah, we're just figuring out how to convince our tooling first :-)

[agocke]

/backport to release/6.0

[github-actions]

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1514956550

[github-actions]

@agocke an error occurred while backporting to release/6.0, please check the run log for details!

Error: @agocke is not a repo collaborator, backporting is not allowed.

[agocke]

/backport to release/6.0

[github-actions]

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1515112560