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

Register Middlewares as Transient to avoid clobber #958

Merged
merged 6 commits into from
Nov 10, 2021
Merged

Register Middlewares as Transient to avoid clobber #958

merged 6 commits into from
Nov 10, 2021

Conversation

gkinsman
Copy link
Contributor

I noticed this issue due to a failing test in JustSayingStack.

Most of our tests only establish a single subscription, which works fine. If there are multiple subscriptions however, then the second will resolve the same instance of LoggingMiddleware or SqsPostProcessorMiddleware (the only middlewares that are resolved from DI). As those instances were already used to build the previous pipeline, they are already part of an existing pipeline, and therefore have _next values that link them to the next step. When the second builder comes along and resolves the same middleware, it redirects the _next to the second subscription. The end result is that the same handler is invoked for all subscriptions. Bad!

This PR does the following to fix this:

  • Adds a test to ensure this doesn't happen
  • Changes the middleware registrations to be transient
  • Emits an InvalidOperationException when constructing middlewares if the _next property has already been populated, which indicates that the middleware has been incorrectly registered and the single instance is already part of an existing pipeline. If there's only one subscription this won't throw, so it still works for apps that only have a single sub and single instance middlewares.

George Kinsman added 2 commits November 10, 2021 16:43
…are pipeline gets its own instance. Without this, when multiple subscriptions are registered, middlewares clobber each other when built.
@gkinsman gkinsman requested a review from a team as a code owner November 10, 2021 17:23
@codecov
Copy link

codecov bot commented Nov 10, 2021

Codecov Report

Merging #958 (48bd39b) into main (1ed1c34) will decrease coverage by 0.09%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #958      +/-   ##
==========================================
- Coverage   84.37%   84.27%   -0.10%     
==========================================
  Files         131      131              
  Lines        3199     3206       +7     
==========================================
+ Hits         2699     2702       +3     
- Misses        500      504       +4     
Flag Coverage Δ
linux 84.27% <66.66%> (-0.10%) ⬇️
macos 53.09% <40.00%> (-0.03%) ⬇️
windows 53.09% <40.00%> (+0.26%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ging/Middleware/Handle/HandlerMiddlewareBuilder.cs 83.78% <42.85%> (-9.77%) ⬇️
...njection.Microsoft/IServiceCollectionExtensions.cs 86.73% <100.00%> (ø)
...ndencyInjection.StructureMap/JustSayingRegistry.cs 100.00% <100.00%> (ø)
...ustSaying/Messaging/Middleware/MiddlewareBase`2.cs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ed1c34...48bd39b. Read the comment docs.

@martincostello martincostello added this to the v7.0.0 milestone Nov 10, 2021
@gkinsman gkinsman merged commit b6b4286 into justeattakeaway:main Nov 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants