Closed bgentry closed 1 month ago
@bgentry Not sure I love this one. I tried to write a short version of why I think Start
/Stop
is better than Run
below, although it still ended up longer than I'd like (sorry about that). The tl;dr though is: how committed are you to this change? I wonder if we can get most of the benefits via a modified approach that uses a little less abstraction and doesn't change the APIs as dramatically.
Here's the short version of why start/stop is good:
The error feedback is quite important. Lack of start up error feedback on start is what led to by far our longest running and hardest to debug (it took me many, many hours) intermittency problem the project's ever had, in the listener. These particular services weren't returning error feedback (yet), but if you look at all the other start/stop implementations, most of them are, and it's reasonable to assume that other services might want to do so in the future.
The Run
paradigm provides no way to differentiate a cancel from a stop. This would make the job completer and client itself impossible to implement for example:
internal/jobcompleter/job_completer.go
254-func (c *BatchCompleter) Start(ctx context.Context) error {
255: stopCtx, shouldStart, stopped := c.StartInit(ctx)
client.go
606-func (c *Client[TTx]) Start(ctx context.Context) error {
607: fetchCtx, shouldStart, stopped := c.baseStartStop.StartInit(ctx)
I'm not sure that we can say that the Run
interface is "simpler, when what used to be this:
require.NoError(t, cleaner.Start(ctx))
Becomes this:
ctx2, cancel2 := context.WithCancel(ctx)
stopCh2 := make(chan struct{})
go func() {
defer close(stopCh2)
cleaner.Run(ctx2)
}()
t.Cleanup(func() { riverinternaltest.WaitOrTimeout(t, stopCh2) })
t.Cleanup(cancel2)
Being able to put stop on a cleanup hook like t.Cleanup(service.Stop)
makes test cases quite pretty, very terse/readable, and fast/easy to write. Start/stop has one more function, but is much easier to understand and use.
Regarding wrappers: I'm not against wrappers if there's good reason for them, but when all they're doing is moving a couple lines out of sight (which then makes those lines harder to find), I'm not super on board. IMO the presence of s.StaggerStart(ctx)
in each service a feature because it lets you easily see exactly how start up works, and lets you jump right into the implementation. If each service unrolled the full stagger start logic so it was duplicated everywhere, that'd be one thing, but instead it's nicely encapsulated into one function call, which seems fine to invoke from multiple places.
I'd also try to consider the proposal here from a distance, like if you were a new programmer just looking into the project. You have each service presenting a Run
interface, but when you're trying to use them you actually use a separate Start
/Stop
interface, and you have a wrapper that converts one to the other. That'd feel kind of weird wouldn't it?
Not going forward with this at the moment.
This attempts to simplify the maintenance service interface to make it more suitable for exposing externally. Rather than requiring the services to be a
startstop.Service
and having each service independently reimplement the semantics of start/stop/closed, these shared bits are extracted into amaintenanceServiceWrapper
type.The maintenance services themselves only need to fit a simpler
MaintenanceService
interface, containing a single methodRun(ctx)
. The wrapper is in charge of fulfilling thestartstop.Service
interface and providing common handling for exit scenarios. Maintenance services must run until the provided context is cancelled, and then must exit promptly afterward. The wrapper bridges these behaviors to provide a blockingStop()
that waits until the service has exited.While this has the effect of making maintenance services no longer fulfill the
startstop.Service
interface, they also do not need to leverage the more complexstartstop.BaseStartStop
type to properly fulfill theService
interface. This is desirable in terms of extensibility and moving toward allowing maintenance processes to be configured and provided externally, because the only type that needs to be exposed here is theMaintenanceService
interface. See the follow up PR for an example of how this could be done.Users or external libraries can provide additional services which implement that one method, and which can then take advantage of being able to run only on the elected leader with automatic start/stop.
This also has the effect of removing the ability of maintenance services to surface an
error
when attempting to start them, however I think this might be a good change. We weren't leveraging this yet anywhere, and I also don't think there's anything useful we could do with these returned errors other than log them. We can still attempt to restart a prematurely exited service if we wish to do so, though at that point the service is not properly implemented.Individual services also do not need to concern themselves with details like
StaggerStartupDisable
because this is now implemented in the wrapper.