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

Use IAsyncConfigureOptions #15125

Closed
wants to merge 1 commit into from
Closed

Conversation

hishamco
Copy link
Member

No description provided.

@MikeAlhayek
Copy link
Member

@hishamco IAsyncConfigureOptions is not the same as IConfigureOptions

With IAsyncConfigureOptions you cannot resolve options using IOptions<>. IAsyncConfigureOptions is to be used for casees where you need to call external services after the service provider is created.

@hishamco
Copy link
Member Author

The PR is still WIP, I will react to the changes ASAP

@hishamco
Copy link
Member Author

IAsyncConfigureOptions is to be used for casees where you need to call external services after the service provider is created.

@MikeAlhayek I assume we should use the options itself

@MikeAlhayek
Copy link
Member

no. you should not use IAsyncConfigureOptions for configuring options. This is only to be used to initialized services that depends on settings not to configure options.

@hishamco
Copy link
Member Author

I already saw the BlobOptions, also this could be doable in the SMTP email service. FYI I already tried it but it fails in the site settings call

@MikeAlhayek
Copy link
Member

yes I expect it to fail because it is called after the SP is already created.

@hishamco
Copy link
Member Author

Any idea how to fix it, because this is something we might want in the email services

@MikeAlhayek
Copy link
Member

@hishamco
IAsyncConfigureOptions does not do the same job of IConfigurationOptions so you cannot replace them as you seems to be trying to do in this PR.

With IConfigurationOptions<HishamOptions> you can inject IOptions<HishamOptions> into your service to get access of the HishamOptions object.

However, with IAsyncConfigureOptions<HishamOptions> you would inject it using HishamOptions. This does not mean let's replace IOptions<HishamOptions> by HishamOptions and just start using IAsyncConfigureOptions.

in the example of BlobOptions you mentioned, we use IAsyncConfigureOptions to asynchronously initialize the blog service. We check whether or not the Blob storage container exists and if not we create a new container "after the ServiceProvider is created". Meaning at that point, we can't inject any additional service.

In your Email service, why do you think you need to use it? Which service that required initialization "not configuration"?

@hishamco
Copy link
Member Author

In your Email service, why do you think you need to use it? Which service that required initialization "not configuration"?

Seems @Piedone wants to use the async version instead of GetAwaiter().GetResult(), correct me if I'm wrong Zoltan

@MikeAlhayek
Copy link
Member

which code are you referring to? It is okay to use GetAwaiter().GetResult() to use in IConfigurationOptions since that is only called during startup. and the GetAwaiter().GetResult() only truly blocks once during the startup cycle. Ideally, Microsoft would provider us IConfigurationAsyncOptions where we don't need to block at all. But at this point, Microsoft are not willing to make Option configuration async so we just have to live with it.

@Piedone
Copy link
Member

Piedone commented Jan 21, 2024

Wait, so IAsyncConfigureOptions<TOptions> is NOT the async equivalent of IConfigureOptions<TOptions>? Given the similar name and method signature, this is really confusing.

I then suggest either one of the following:

  • Rename the service, to like IAsyncConfigurationProvider and in its interface docs emphasize the similarity but differences to IConfigureOptions<TOptions>.
  • Make it more similar by adding an IAsyncOptions<T> type to inject, with GetValueAsync(). This would otherwise work the same as IOptions<T> and its Value property.

@hishamco
Copy link
Member Author

Ya, it confused me too :)

@hishamco
Copy link
Member Author

So, Let us do it after merging Azure Email because it will take long time :)

@MikeAlhayek
Copy link
Member

Yes @Piedone it is confusing. I had the same argument with JT when he introduced it. It may not be a bad idea to rename it. It's for initializing services not configuring options using IOption pattern.

@hishamco
Copy link
Member Author

Is this solve a real problem Mike? just kidding :)

@MikeAlhayek
Copy link
Member

Refactoring sometimes makes sense. And sometimes is a must. But, it's typically risky to take on specially if there is no testing to ensure that the end result is the same.

Either way, PRs should be dealt with fast. Either we request changes, close it or merge it. As previously mention a PR that takes a long time in the queue will likely mean it adds to real value

@hishamco
Copy link
Member Author

hishamco commented Jan 22, 2024

@MikeAlhayek could you please close this and handle Zoltan's comments #15125 (comment) if you have time, while you know the differences between both APIs

Meanwhile I'm trying to updating and finalizing my PRs which are a lot :)

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.

3 participants