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

Initialize the RequestLocalizationMiddleware asynchronously #14784

Closed
wants to merge 1 commit into from

Conversation

MikeAlhayek
Copy link
Member

@MikeAlhayek MikeAlhayek commented Nov 28, 2023

Fix #14798

@hishamco
Copy link
Member

Please hold on, I have another PR related to localization from a week or so, I need to do a final test before I merge it, and then I can look into your

@MikeAlhayek
Copy link
Member Author

@hishamco this PR fixes a one issue that is causing thread locking on every single request. I am not sure what does you PR do, but I am not sure we should block this PR meanwhile.

@jtkech
Copy link
Member

jtkech commented Nov 30, 2023

Feel wrong to me, I think we can find a way to configure the options asynchronously without having to clone the whole middleware.

Hmm the "problem" with an initializer is that it runs early, here we configure later on, for info I already suggested to add ConfigureAsync() to our startup, would be useful here, maybe time to do it, or as said find another compromise.

@hishamco
Copy link
Member

for info I already suggested to add ConfigureAsync() to our startup, would be useful here, maybe time to do it, or as said find another compromise.

I remember Seb already talked about it previously, so @MikeAlhayek I think we need to hold this PR for the moment if you don't mind

@jtkech I'm with you to have it in our startup

@hishamco
Copy link
Member

Regarding my first comment I thought you added ConfigureAsync() but after I review now, I might disagree with the PR, I prefer to have ConfigureAsync() in our startup as JT mentioned

@MikeAlhayek
Copy link
Member Author

Feel wrong to me, I think we can find a way to configure the options asynchronously without having to clone the whole middleware.

@jtkech I don't disagree with you here. I just don't think we have better option at this point. If you are going to add ConfigureAsync, then sure we don't need to merge this PR and we can use the ConfigureAsync.

@hishamco
Copy link
Member

Please let us don't merge this for now

@jtkech jtkech mentioned this pull request Dec 1, 2023
@MikeAlhayek MikeAlhayek closed this Dec 1, 2023
@MikeAlhayek
Copy link
Member Author

Closing in favor of #14801

@hishamco hishamco deleted the ma/load-localization branch December 1, 2023 00:35
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.

Initialize the RequestLocalizationMiddleware asynchronously
3 participants