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

[WIP] Update entry assembly #100935

Closed
wants to merge 2 commits into from

Conversation

fanyang-mono
Copy link
Member

No description provided.

Copy link
Contributor

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


}

INT32 Assembly::ExecuteMainMethod(PTRARRAYREF *stringArgs, BOOL waitForOtherThreads)
Copy link
Member

Choose a reason for hiding this comment

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

What are you trying to achieve by this refactoring?

Copy link
Member

@steveisok steveisok Apr 12, 2024

Choose a reason for hiding this comment

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

What we are trying to determine is the best place to allow swapping the entry assembly and it having a real effect. The goal in #99883 is to allow some / all of the TPA assemblies to load, sleep/wake up, and allow the entry assembly to be swapped.

We've identified two ways to do this. Make the runtime aware, which is what this PR is attempting to do. And the second is another export in the host that doesn't shut the runtime down after it's done (the onus would be put on the custom host to do that).

Copy link
Member

Choose a reason for hiding this comment

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

Alternative approach we've discussed in the past is this:

  • Rely on startup hook to "stop" the runtime and wait for knowing the final entry point assembly
  • Add a new API which sets the entry point assembly for all of the other places where the runtime uses this information
  • Rely on reflection to load an execute the entry point assembly from the startup hook.
  • Basically the original entry point assembly would never be executed.

This is more of the "do this in managed code" approach. Not sure how this would fit with the host changes to allow for TPA modifications after startup.

There are other similar ways - for example the above could be done without startup hooks, where the original Main would perform the operations and effectively "replace itself".

Copy link
Member

Choose a reason for hiding this comment

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

  • Rely on reflection to load an execute the entry point assembly from the startup hook.
  • Basically the original entry point assembly would never be executed.

The way the runtime appears to work right now is that the original main will be executed regardless of what you do in the hook. Also, aren't there limitations executing code from the hook?

Copy link
Member

Choose a reason for hiding this comment

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

The way the runtime appears to work right now is that the original main will be executed regardless of what you do in the hook.

Yes - but nothing is stopping the hook from calling Environment.Exit ;-)
I think the idea of doing this from the main is better anyway. That's what dotnet watch uses for ASP.NET anyway. BTW that would be another interesting thing to look at - they're one of the teams asking for the entry point replacement as the watch breaks some apps which rely on it.

Also, aren't there limitations executing code from the hook?

Not really - AFAIK there are no restrictions other than "you should not do that" - but mostly those are there to not break the "Main", but if the "Main" is empty then they're moot.

Copy link
Member

@jkotas jkotas Apr 12, 2024

Choose a reason for hiding this comment

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

Not sure how this would fit with the host changes to allow for TPA modifications after startup.

TPA additions after startup can be done via the AssemblyLoadContext.Default.

I do not think we should be enabling modifications of TPA after startup. It would be a pit of failure when an assembly that wanted to be overridden by the copy in the app got loaded too early. Instead, we should figure out how to startup the runtime with TPA subset that is never going to be modified.

Copy link
Member Author

@fanyang-mono fanyang-mono Apr 15, 2024

Choose a reason for hiding this comment

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

If we take the Main approach, then the real app main could be invoked by reflection. I wonder what is the need of creating a new API Assembly.SetEntryPoint? It seems that what Functions team would like to do could be achieved with existing API's. Am I missing anything?

Copy link
Member

Choose a reason for hiding this comment

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

Without the new API for example this Assembly.GetEntryAssembly would return wrong value (it would point to the "host's template Main"). I think there are other places that this shows up in some way or form as well. Some apps use this to mostly get to the location of app's files (Assembly.GetEntryAssembly().Location) for example.

Copy link
Member

Choose a reason for hiding this comment

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

ASP.NET Core has number of places that call Assembly.GetEntryAssembly: https://github.com/search?q=repo%3Adotnet%2Faspnetcore+GetEntryAssembly&type=code

As Vitek said, it is important for Assembly.GetEntryAssembly to return the actual application assembly to avoid breaking these places.

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha!

@fanyang-mono
Copy link
Member Author

Closing this PR, as we chose not to proceed with this approach.

We found that this approach performed the same as the Main approach (no runtime change needed) mentioned in the discussion above.

@github-actions github-actions bot locked and limited conversation to collaborators Jun 15, 2024
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.

4 participants