Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Add host context-based entry points for native hosting#5859

Merged
elinor-fung merged 25 commits into
dotnet:masterfrom
elinor-fung:hostContext
May 2, 2019
Merged

Add host context-based entry points for native hosting#5859
elinor-fung merged 25 commits into
dotnet:masterfrom
elinor-fung:hostContext

Conversation

@elinor-fung
Copy link
Copy Markdown
Member

@elinor-fung elinor-fung commented Apr 15, 2019

  • Track existing hostpolicy_context in hostpolicy
  • Add host_context to hostfxr to represent active (first) and secondary contexts
  • Switch com/ijw/winrt hosts to use host context-based APIs
  • Update nativehost test executable to exercise new APIs
  • Make non-context-based entry points check for existing context / create empty context

hostfxr API surface changes:

  • New exports:
    • hostfxr_initialize_for_app
    • hostfxr_initialize_for_runtime_config
    • hostfxr_run_app
    • hostfxr_get_runtime_property_value
    • hostfxr_set_runtime_property_value
    • hostfxr_get_runtime_properties
    • hostfxr_close
  • Change hostfxr_get_runtime_delegate signature to use host context

hostpolicy API surface changes:

  • New exports:
    • corehost_initialize
  • Remove corehost_get_coreclr_delegate

Failure handling:

  • coreclr initialization failure will cause all future attempts to initialize to fail.
  • hostpolicy library loaded for the first attempt is used for all subsequent ones even if the first failed initialization

Mixed usage of non-context-based and context-based entry points:

  • Non-context-based entry points create an 'empty' active context after loading hostpolicy. There is no guarantee that hostpolicy has been initialized and the runtime loaded at this point.
  • Getting properties on the 'empty' active context (i.e. passing nullptr to hostfxr_get_runtime_property_value or hostfxr_get_runtime_properties) is not supported
  • On initializing a secondary context when the active context is 'empty' hostfxr passes a flag to hostpolicy corehost_initialize to indicate that it requires waiting for initialization (through a different request) to complete
  • If that other initialization fails, the request that was waiting for it to complete also fails.

Not handled in this:

  • Hosts should reuse existing hostfxr library if it is already loaded
  • Check that hostpolicy policy requested for load is the same as the already loaded one if it exists (host commands, which don't go through all the same context things since they don't load the runtime)
  • Compatibility of secondary contexts
    • Failure on incompatible runtime / frameworks
    • Warning on new properties or ones with different values
  • Automated tests

@elinor-fung elinor-fung force-pushed the hostContext branch 7 times, most recently from 1f06d7a to bbc65d3 Compare April 16, 2019 21:04
@vitek-karas
Copy link
Copy Markdown
Member

I quickly scanned through it and it looks good, I'll do the thorough review later on.

One request: Could you please separate the "Fail early on duplicate properties" into its own PR? (I think about it as a pre-req for this work, and it should make this PR slightly smaller)

@elinor-fung
Copy link
Copy Markdown
Member Author

Separated into: #5898

Comment thread src/corehost/error_codes.h Outdated
Comment thread src/corehost/cli/hostfxr.h
Comment thread src/corehost/cli/hostfxr.h Outdated
Comment thread src/corehost/cli/fxr_resolver.h Outdated
Comment thread src/corehost/cli/context_contract.h Outdated
Comment thread src/corehost/cli/context_contract.h Outdated
@elinor-fung elinor-fung force-pushed the hostContext branch 3 times, most recently from 72cf00e to fc34c2c Compare April 23, 2019 05:15
Comment thread src/corehost/cli/fxr/fx_muxer.cpp
Comment thread src/corehost/cli/fxr/fx_muxer.cpp
Comment thread src/corehost/cli/fxr/fx_muxer.cpp Outdated
Comment thread src/corehost/cli/fxr/fx_muxer.cpp
Comment thread src/corehost/cli/fxr/fx_muxer.cpp
Comment thread src/corehost/cli/fxr/hostfxr.cpp Outdated
Comment thread src/corehost/cli/hostpolicy/hostpolicy.cpp Outdated
Comment thread src/corehost/cli/fxr/hostfxr.cpp
Comment thread src/corehost/cli/fxr/hostfxr.cpp Outdated
Comment thread src/corehost/cli/hostpolicy/hostpolicy.cpp Outdated
Comment thread src/corehost/cli/hostpolicy/hostpolicy.cpp
Comment thread src/corehost/cli/hostpolicy/hostpolicy.cpp Outdated
Comment thread src/corehost/cli/hostpolicy/hostpolicy.cpp Outdated
Comment thread src/corehost/cli/hostpolicy/hostpolicy.cpp Outdated
Comment thread src/corehost/cli/hostpolicy/hostpolicy.cpp Outdated
Comment thread src/corehost/cli/hostpolicy/hostpolicy_context.cpp Outdated
Comment thread src/corehost/cli/fxr/fx_muxer.cpp Outdated
Comment thread src/corehost/cli/fxr/fx_muxer.cpp Outdated
Comment thread src/corehost/cli/hostpolicy/hostpolicy.cpp Outdated
Comment thread src/corehost/cli/fxr/fx_muxer.cpp Outdated
@elinor-fung elinor-fung force-pushed the hostContext branch 3 times, most recently from 948d6e8 to 32c2885 Compare April 27, 2019 02:07
@elinor-fung elinor-fung changed the title [WIP] Add host context-based entry points for native hosting Add host context-based entry points for native hosting Apr 27, 2019
@elinor-fung
Copy link
Copy Markdown
Member Author

elinor-fung commented Apr 27, 2019

I think this should be in a reasonable state now.

Things that I would like to keep separate and do in follow-up changes (in an attempt to break things up and keep this review a little smaller...):

  • Validate compatibility of secondary contexts
    • i.e. that existing TODO - there are TODOs in the two places where I expect to add the checks
  • Reuse already loaded hostfxr library in hosts
  • Check hostpolicy library requested for load is the same as the existing one if it is already loaded (host commands, which don't go through all the same context things since they don't load the runtime)
    • existing behaviour (without the changes in this PR) is to just reuse what is loaded without any checks or indication if it is not the same one.
  • Automated tests
    • the nativehost test exe is updated to exercise the new entry points for basic scenarios
    • non-happy-path scenarios are not yet hit (e.g. failure cases, concurrent requests)

Comment thread src/corehost/cli/fxr/hostfxr.cpp
Comment thread src/corehost/cli/fxr/hostfxr.cpp Outdated
Copy link
Copy Markdown
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

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

I think I'm done reviewing the "Scenarios". That is thinking through the various paths which should work.
I'll need to do another pass on the whole thing (for nits and so on)....

One request: Can you please add the test automation to this change (and not do it in a followup)? Even if there's small test coverage I think it's important to have at least something in place from the get go, to avoid big breaks.

Comment thread src/corehost/cli/corehost_context_contract.h
Comment thread src/corehost/cli/hostpolicy/hostpolicy.cpp Outdated
Comment thread src/corehost/cli/hostpolicy/hostpolicy.cpp Outdated
Comment thread src/corehost/cli/fxr/fx_muxer.cpp
Comment thread src/corehost/cli/hostpolicy/hostpolicy.cpp
Comment thread src/corehost/cli/fxr/fx_muxer.cpp
Comment thread src/corehost/cli/fxr/fx_muxer.cpp
Comment thread src/corehost/cli/fxr/fx_muxer.cpp Outdated
Comment thread src/corehost/cli/fxr/fx_muxer.cpp
Comment thread src/corehost/cli/fxr/fx_muxer.cpp
hostpolicy internally tracks its context and external functions just
operate on the one context and error out if it is not in the expected
state for that operation
Placeholder TODOs for compatibility checks
Comments
…tion

request (which may not yet have started) to complete
@elinor-fung
Copy link
Copy Markdown
Member Author

Basic tests are in the latest commit. They only hit the most basic 'happy' path right now - not hitting failures or concurrent requests.

Copy link
Copy Markdown
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

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

We're almost there - this looks good. I only quickly looked through the tests since we know we need to do more work there, but the product changes look solid.

Comment thread Documentation/design-docs/hosting-layer-apis.md Outdated
Comment thread src/corehost/cli/corehost_context_contract.h Outdated
Comment thread src/corehost/cli/corehost_context_contract.h
Comment thread src/corehost/cli/fxr/fx_muxer.cpp
Comment thread src/corehost/cli/fxr/fx_muxer.cpp Outdated
Comment thread src/corehost/cli/fxr/hostfxr.cpp Outdated
Comment thread src/corehost/cli/hostpolicy/hostpolicy.cpp Outdated
Comment thread src/corehost/cli/hostpolicy/hostpolicy.cpp Outdated
Comment thread src/corehost/cli/fxr/fx_muxer.cpp
Comment thread src/corehost/cli/fxr/fx_muxer.cpp Outdated
corehost_main_with_output_buffer_fn host_main = nullptr;

int code = load_hostpolicy(impl_dll_dir, &corehost, host_contract, "corehost_main_with_output_buffer", &host_main);
int code = load_hostpolicy(impl_dll_dir, &corehost, hostpolicy_contract, "corehost_main_with_output_buffer", &host_main);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just a worry:
I guess this already exists as a problem today... but still.
If somebody calls this - it will load hostpolicy.
After that it's possible to call some other entry point which will also load hostpolicy, potentially a different one.
If it's the same one, we're probably fine since this calls unload.
If it's a different one, not sure how OS handles it... maybe it just works (two different dlls with the same name)

In any case, since this doesn't perform any synchronization we rely on the caller to not mix/match.

Probably a non-issue... just wanted to raise this for awareness.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I added a log line for if hostpolicy was already loaded, the directory it was loaded from, and the requested directory (but did not change the existing behaviour).

Rename corehost -> hostpolicy_dll
Comment thread src/corehost/cli/fxr/fx_muxer.cpp Outdated
@vitek-karas
Copy link
Copy Markdown
Member

There's another related issue - component hosts (comhost, winrthost, ijwhost) try to load hostfxr from the their folder first - basically trying to support self-contained behavior. I think this is wrong.

I filed a separate issue #6236 for this, but feel free to fix it in this PR if you want (although it's probably a relatively non-trivial change, since it needs different defines and so on... nethost is currently treated the same as comhost, which will need to change).

@elinor-fung
Copy link
Copy Markdown
Member Author

Switched to blocking initializing for self-contained components and will plan handle 6236 in a separate change.

@elinor-fung elinor-fung added this to the 3.0 milestone May 2, 2019
@elinor-fung elinor-fung merged commit 4f7fefe into dotnet:master May 2, 2019
@elinor-fung elinor-fung deleted the hostContext branch May 2, 2019 18:36
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…etup#5859)

- Track existing hostpolicy_context in hostpolicy
- Add host_context to hostfxr to represent active (first) and secondary contexts
- Switch com/ijw/winrt hosts to use host context-based APIs
- Update nativehost test executable to exercise new APIs
- Make non-context-based entry points check for existing context and create an empty context
- Basic automated tests for context-based entry points

Commit migrated from dotnet/core-setup@4f7fefe
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants