Better transport seam integration to enable dependency injection #4193
Conversation
danielmarbach
force-pushed
the
transport-seam-di
branch
4 times, most recently
from
fbad0a5 to
5927123
Compare
danielmarbach
force-pushed
the
transport-seam-di
branch
2 times, most recently
from
bac749d to
aed1e60
Compare
danielmarbach
changed the title
danielmarbach
force-pushed
the
transport-seam-di
branch
from
aed1e60 to
00f29f3
Compare
…ers to leverage new DI capabilities
danielmarbach
force-pushed
the
transport-seam-di
branch
from
00f29f3 to
7a0ca15
Compare
| using Microsoft.Extensions.Hosting; | ||
|
|
||
| public interface IProvideQueueLength | ||
| public interface IProvideQueueLength : IHostedService |
There was a problem hiding this comment.
I feel that the implementers need to decide whether using IHostedService is the right thing to do.
in other words, should this interface not implement IHostedService ?
There was a problem hiding this comment.
That's an excellent question. I was going down that path because the existing code and test was starting and stopping things to make it work. By not adding the hosted service interface the test code would become slightly more complex because it would have to retrieve the AbstractQueueLengthProvider and then start that. This would also mean we then technically need three registrations of which one would only ever be used by the transport tests. We can't just retrieve the IHostedService instance because while there is currently only one, there might be more later. So I'm kind of torn. I'll merge the PR and think about this a bit more.
There was a problem hiding this comment.
Yeah, I can see the complexity.
TBH, given this is not a public API, I am fine with it 😄
| stop = new CancellationTokenSource(); | ||
|
|
||
| poller = Task.Run(async () => | ||
| while (!stoppingToken.IsCancellationRequested) |
There was a problem hiding this comment.
Would the PeriodicTimer be a better fit for this background task?
I used it in
I am just wondering.
There was a problem hiding this comment.
And yes, I noticed this is existing code and they are all the same pattern!
There was a problem hiding this comment.
Yeah the reason I use the while with the delay pattern is because the existing code was already doing that. At least from that perspective I wanted to stay compatible. I was aware the period timer pattern exists. It is also documented here https://learn.microsoft.com/en-us/aspnet/core/fundamentals/host/hosted-services?view=aspnetcore-8.0&tabs=visual-studio#asynchronous-timed-background-task
Can you elaborate on the "feels nicer" part?
There was a problem hiding this comment.
Can you elaborate on the "feels nicer" part?
There's not much to elaborate on; I guess, it feels shorter and more to the point.
But this is just my opinion, nothing wrong with the other approach either.
…ers to leverage new DI capabilities (#4193) Co-authored-by: danielmarbach <danielmarbach@users.noreply.github.com>
DavidBoike
changed the title
This PR is the outcome of a discussion related to changes done as part of #3965 which had to introduce a
ThroughputQueryProvidertype on the transport customization. By aligning the transport customization integration well enough with how the persistence seam integrates into the DI container it becomes possible to register types directly that then the transport infrastructure can resolve. This is demonstrated by migrating the queue length query providers to actual hosted services.