Skip to content
This repository was archived by the owner on Nov 7, 2018. It is now read-only.

Regression: static options snapshots are no longer correctly cached #211

Closed
kevinchalet opened this issue Jul 2, 2017 · 5 comments
Closed

Comments

@kevinchalet
Copy link

kevinchalet commented Jul 2, 2017

When #199 was fixed, you guys opted for making the options snapshots scoped, which introduced a terrible regression.

Now, configuration delegates are run for every authentication handler and for every request, which has both performance and functional implications in ASP.NET Core projects like the OIDC client middleware and community projects like the OIDC server I develop.

Please revert this design change.

More context:

@kevinchalet
Copy link
Author

/cc @HaoK

@HaoK
Copy link
Member

HaoK commented Jul 3, 2017

This wasn't a regression on the options side, this was intentional to make snapshots scoped, Monitor/IOptions are singletons, the issue tracking whether security should be switch to monitor is here: aspnet/Security#1282

@HaoK HaoK closed this as completed Jul 3, 2017
@HaoK HaoK added the duplicate label Jul 3, 2017
@kevinchalet
Copy link
Author

kevinchalet commented Jul 3, 2017

Call that intentional if you want, but it impacts even built-in ASP.NET Core components like the OIDC middleware, that has to retrieve the configuration metadata before each OIDC operation (login, logout, etc.) because the ConfigurationManager can't be shared.

It worked fine in preview1. Am I the only one thinking that introducing such a breaking change in the options stack in a public preview without fixing the components that rely on the previous behavior is... extremely weird?

@HaoK
Copy link
Member

HaoK commented Jul 3, 2017

There's no test contract today, we don't have any tests that depend on this behavior in Security, if it causes functional failures as opposed to perf issues, we need to have tests that enforce this.

@Tratcher does OIDC configuration require reusing the same configuration instances, or can each request recreate these?

@ajcvickers
Copy link

@Eilon Copying you per discussion today.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants