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

Make IPushMessages.Start task returning #5447

Open
andreasohlund opened this issue Oct 4, 2019 · 2 comments
Open

Make IPushMessages.Start task returning #5447

andreasohlund opened this issue Oct 4, 2019 · 2 comments

Comments

@andreasohlund
Copy link
Member

Since it's called in the async part of endpoint lifecycle. It also aligns with the Stop method that already is async.

Also consider starting the pumps concurrently just like we stop them concurrently

@danielmarbach
Copy link
Contributor

@andreasohlund haven't we chewed through this argument around V6 over a dozen times and always came back to the same conclusion that Start doesn't need to return a task. The main argument was it encourages the pump design where the spinning/loop/receive operations are pushed to the background. Another argument that came up many times was that any IO-bound operation that is executed in the start slows down the endpoint start. Stop not returning task forces you to initialize the IO bound stuff in the background to speed up start. Of course this comes with a cost that start problems cannot be reported back to the caller. But the risk can be mitigated by either using Prestartup Checks or do a warmup in the init if required. All major transport today operate in that way and don't require an async start. See for example

https://github.com/Particular/NServiceBus.SqlServer/blob/master/src/NServiceBus.SqlServer/Receiving/MessagePump.cs#L64
https://github.com/Particular/NServiceBus.AmazonSQS/blob/master/src/NServiceBus.AmazonSQS/MessagePump.cs#L117

Don't get me wrong I'm not against it. I just think the explanation in this issue doesn't take the history into account of the decision making we did at that time. I think it should and clearly argue why the change is required

@andreasohlund
Copy link
Member Author

I didn't remember that discussion :) I was just surprised that Stop was task returning and start was not. It also felt off that we did "sync work" in the async part of the endpoint startup and that Init that is called before start is async

Looking at the code I'm starting to wonder if this is a sign that we should merge the current Init (which is async) into a new async Start()? (not sure why we needed an Init in the first place?)

Lets sync up on a call and hash this out, I'll schedule something

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

No branches or pull requests

2 participants