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

Restore default request service feature after middleware completes #2686

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

danielmarbach
Copy link
Contributor

@danielmarbach danielmarbach commented Sep 6, 2024

Issue describing the changes in this PR

During a review of a problem one of my client faces I dug deeper into the middleware and was wondering why the request service feature is not restored after the pipeline executed. This implementation seems to be more correct and avoids the context scope to leak into other middlewares

Pull request checklist

  • My changes do not require documentation changes
    • Otherwise: Documentation issue linked to PR
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • I have added all required tests (Unit tests, E2E tests)

Additional information

Additional PR information

@danielmarbach danielmarbach marked this pull request as ready for review September 6, 2024 09:37
@danielmarbach danielmarbach changed the title Middleware improvement Restore default request service feature after middleware completes Sep 6, 2024
@liliankasem
Copy link
Member

@danielmarbach do you mind rebasing main so we can run the CI? Thanks!

@danielmarbach
Copy link
Contributor Author

Done

FYI I have previously already set

Allow edits by maintainers

@liliankasem
Copy link
Member

/azp run dotnet-worker.public

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@liliankasem liliankasem requested a review from jviau October 16, 2024 20:14
@fabiocav
Copy link
Member

@danielmarbach have you validated what was stated above? The scope here should not lead to middleware outside of this context. If you've observed that, can you share a repro?

The disposal here doesn't hurt, but will also be a no-op, as the feature is initialized with an externally provided service provider.

@danielmarbach
Copy link
Contributor Author

Like I mentioned above, I reviewed the function code to troubleshoot an issue we saw with a client code. During that review, I stumbled over this code and was thinking that the way this is structured it could lead to the context scope leaking. I wrote a quick test to verify that, which failed, and then added a potential fix for it. I'm not attached to this code. If you feel it is not worth pulling this in that's totally fine for me but I'm also not investing more time into it either ;)

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