Enable calling get_runtime_delegate from app context #37473

rseanhall · 2020-06-05T05:29:50Z

This is the first part of implementing #35465. This pull request is supposed to address all the complexities of allowing hostfxr_get_runtime_delegate to be called from a context from hostfxr_initialize_for_dotnet_command_line. As designed in #36990, an app context may only ask for hdt_load_assembly_and_get_function_pointer. Implementing the new hdt_get_function_pointer type will be done in a subsequent pull request after this one is merged.

As part of this change, there are a couple of behavior changes that may technically be breaking changes.

Loading the runtime when given an invalid delegate type

In 3.x, hostfxr_get_runtime_delegate doesn't error out when given an invalid delegate type. This means that it will load the runtime before calling into hostpolicy. Then the get_delegate call in hostpolicy is where it errors out. This behavior is unacceptable because the native host needs to have control over starting the runtime if there's no chance of getting a delegate from hostfxr.

In order to implement this change, I started making hostpolicy return the highest delegate type it knows about. Then hostfxr will check against that (or hdt_load_assembly_and_get_function_pointer if it's an old hostpolicy) before trying to load the runtime.

Error code when trying to load SCD component

In 3.x, hostfxr_initialize_for_runtime_config returns the error code InvalidConfigFile when the config is for an SCD component. This error code is misleading since the config file is valid, just not currently supported by hostfxr. I made hostfxr return the new HostApiUnsupportedScenario error code in this case to give the native host better error information.

ghost · 2020-06-05T05:29:55Z

Tagging subscribers to this area: @vitek-karas, @swaroop-sridhar
Notify danmosemsft if you want to be subscribed.

vitek-karas · 2020-06-05T09:09:27Z

Can you please fix the design doc in this PR as well?. In this section it states that the hdt_load_assembly_and_get_function_pointer is not allowed on "app contexts" - but later on it says it is allowed. I think it's fine to allow it, we just need to fix the doc.

vitek-karas · 2020-06-05T09:16:32Z

Re breaking changes:

Loading the runtime when given an invalid delegate type

I think this is good - you're right that we should not do any work if we know it's going to fail. So I like this change in general (I have yet to look at the implementation details).
As for the risk of taking this - I think it's very small, we still fail, and we avoid loading runtime. This is very unlikely to break people (not counting that it's very unlikely that anybody released an app which calls the API with the wrong value as it would not work anyway).

Error code when trying to load SCD component

I'm not sure about this one. I do agree that the returned value is not great (misleading to say the least). That said I know about several cases where the exact exist code is used by calling code to make decisions. So changing it feels risky to me. If this would be a change in the runtime I would not worry too much as we're OK taking small breaking changes in major releases (with good reason). Unfortunately this is a change in hostfxr which doesn't really have the same freedom - it's global and callers always end up calling the latest version. So even minor breaking changes are a problem.

I think we should not make this change - I know the number value is bad, but we at least print out the right error message.

rseanhall · 2020-06-05T09:25:58Z

Error code when trying to load SCD component

The reason I thought it was acceptable is because there is no way for the native host to distinguish this from the case where the config file is actually invalid. So I think their code was brittle to begin with. But I understand if we can't take this change.

I'm also making hostfxr_get_runtime_delegate return the new HostApiUnsupportedScenario error code when called with an app context requesting an unsupported delegate. Is that OK?

vitek-karas · 2020-06-05T09:29:57Z

I'm also making hostfxr_get_runtime_delegate return the new HostApiUnsupportedScenario error code when called with an app context requesting an unsupported delegate. Is that OK?

For some reason I think it is more OK than the other 🤷. This one is a clear programmer error, so it's unlikely there will be fallback code paths which react to the error code to do something meaningful. The first one is data driven - depends on the .runtimeconfig.json so it's frequently out of control for the code itself and thus the code might have a fallback code path.

So yes - I'm OK taking the change on hostfxr_get_runtime_delegate.

src/installer/corehost/cli/corehost_context_contract.h

src/installer/corehost/cli/fxr/fx_muxer.cpp

src/installer/corehost/cli/hostpolicy/hostpolicy.cpp

src/installer/corehost/cli/fxr/standalone/hostpolicy_resolver.cpp

src/installer/corehost/error_codes.h

src/installer/corehost/cli/fxr/hostfxr.cpp

src/installer/corehost/cli/corehost_context_contract.h

src/installer/corehost/cli/hostpolicy/hostpolicy.cpp

vitek-karas

Looks good to me - thanks a lot for the change!
Please wait a day or two for @AaronRobinsonMSFT and @elinor-fung to have a change to take a look as well.

elinor-fung

Thank you!

src/installer/corehost/cli/fxr/fx_muxer.cpp

…gate types. This is required so that the runtime isn't loaded if the request has no chance of succeeding. Fix spelling of initialization_options_t. Add context_contract_version_set flag to initialization_options_t. This allows hostpolicy to know whether it can rely on the version field of corehost_context_contract to be valid. Also always initialize corehost_context_contract to {}.

Currently, it may only request the load_assembly_and_get_function_pointer delegate. Fix discrepancy in design doc. Add error code HostApiUnsupportedScenario for blocks that might be removed. This gives the native host better information on why the request failed. Add tests for running a component from an app context.

rseanhall · 2020-06-09T06:56:34Z

I added the comment about load_assembly_and_get_function_pointer and squashed this into two commits. If you want to see all the individual commits, they're at https://github.com/rseanhall/runtime/tree/35465-app-get-runtime-delegate-3.

vitek-karas · 2020-06-09T11:47:50Z

@rseanhall thanks a lot for the changes!

Dotnet-GitSync-Bot added the area-Host label Jun 5, 2020

rseanhall force-pushed the 35465-app-get-runtime-delegate branch from a195f94 to a2011c2 Compare June 5, 2020 09:36

vitek-karas reviewed Jun 5, 2020

View reviewed changes

AaronRobinsonMSFT reviewed Jun 5, 2020

View reviewed changes

src/installer/corehost/cli/fxr/hostfxr.cpp Show resolved Hide resolved

vitek-karas reviewed Jun 7, 2020

View reviewed changes

src/installer/corehost/cli/corehost_context_contract.h Show resolved Hide resolved

src/installer/corehost/cli/hostpolicy/hostpolicy.cpp Outdated Show resolved Hide resolved

vitek-karas reviewed Jun 7, 2020

View reviewed changes

src/installer/corehost/cli/hostpolicy/hostpolicy.cpp Outdated Show resolved Hide resolved

vitek-karas approved these changes Jun 7, 2020

View reviewed changes

vitek-karas requested a review from elinor-fung June 7, 2020 20:37

rseanhall force-pushed the 35465-app-get-runtime-delegate branch from ac67575 to 9c8da06 Compare June 7, 2020 23:17

AaronRobinsonMSFT approved these changes Jun 8, 2020

View reviewed changes

swaroop-sridhar approved these changes Jun 8, 2020

View reviewed changes

elinor-fung approved these changes Jun 8, 2020

View reviewed changes

src/installer/corehost/cli/fxr/fx_muxer.cpp Show resolved Hide resolved

rseanhall added 2 commits June 9, 2020 16:47

rseanhall force-pushed the 35465-app-get-runtime-delegate branch from 9c8da06 to c993c7f Compare June 9, 2020 06:50

vitek-karas merged commit 3b5a51a into dotnet:master Jun 9, 2020

This was referenced Jun 10, 2020

Implement hdt_get_function_pointer #37696

Merged

Refactor ComponentActivator #37784

Merged

rseanhall mentioned this pull request Jun 17, 2020

Move hostfxr API comments into header file #38019

Merged

ghost locked as resolved and limited conversation to collaborators Dec 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable calling get_runtime_delegate from app context #37473

Enable calling get_runtime_delegate from app context #37473

rseanhall commented Jun 5, 2020 •

edited

Loading

ghost commented Jun 5, 2020

vitek-karas commented Jun 5, 2020

vitek-karas commented Jun 5, 2020

rseanhall commented Jun 5, 2020

vitek-karas commented Jun 5, 2020

vitek-karas left a comment

elinor-fung left a comment

rseanhall commented Jun 9, 2020

vitek-karas commented Jun 9, 2020

Enable calling get_runtime_delegate from app context #37473

Enable calling get_runtime_delegate from app context #37473

Conversation

rseanhall commented Jun 5, 2020 • edited Loading

Loading the runtime when given an invalid delegate type

Error code when trying to load SCD component

ghost commented Jun 5, 2020

vitek-karas commented Jun 5, 2020

vitek-karas commented Jun 5, 2020

rseanhall commented Jun 5, 2020

vitek-karas commented Jun 5, 2020

vitek-karas left a comment

Choose a reason for hiding this comment

elinor-fung left a comment

Choose a reason for hiding this comment

rseanhall commented Jun 9, 2020

vitek-karas commented Jun 9, 2020

rseanhall commented Jun 5, 2020 •

edited

Loading