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

Proposal - Add Async PreInitAsync to allow async operations such as configuration I #708

Closed
wants to merge 4 commits into from

Conversation

damianh
Copy link
Contributor

@damianh damianh commented Jul 25, 2020

Issue #, if available:

A proposal to address #702

Description of changes:

  • Add virtual member PreInitAsync to AbstractAspNetCoreFunction that allows async operations to happen, such as loading configuration from secrets manager, before the webhost is initialized. (I'm not too fond of the name. I'd prefer just InitAsync() and rename Init() to InitWebHost() but that would be a breaking change...)
  • It is only invoked when StartupMode is set FirstRequest which in an asynchronous call stack. It is not possible to call it when StartupMode is set to Constructor as invoking async methods in a constructor is an anti-pattern. (If there is no specific advantage to using Constructor startup mode, could it be argued to remove it? 🤷 )
  • Add test to assert PreInitAsync() is called. I also noticed there was no tests / test coverage for StartMode.FirstRequest; this test now covers it.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@normj
Copy link
Member

normj commented Sep 10, 2020

I think having this only being invoked if StartupMode is set to FirstRequest, which isn't the default, is going to cause too much confusion.

Since users are forced to make async calls in an sync call right now if they need to load things async like secret manager should we just move the GetAwaiter().GetResult() to the AbstractAspNetCoreFunction and call the PreInit method at the same location Init is called. In the default case Task.CompletedTask is returned so that won't affect performance.

@damianh
Copy link
Contributor Author

damianh commented Sep 21, 2020

Ok, will attempt your suggestion.

…n before the webhost is initialized.

It is only invoked when StartupMode is FirstRequest which in an asynchronous call stack.
…stRequest.

I notices there was no test coverage over initializing the WebHost with StartupMode set to FirstRequest.
Need a reference to the lamba function to assert it.
@damianh
Copy link
Contributor Author

damianh commented Dec 27, 2020

Added a comment regarding this to original issue #702 (comment). Not really happy with this proposal as is so going to close.

@damianh damianh closed this Dec 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants