Follow-up from #3601 (comment).
Problem
node.Node.StartService unconditionally defer cancel()s the node-level context as soon as a service's Run returns:
https://github.com/NethermindEth/juno/blob/main/node/node.go#L729-L744
func (n *Node) StartService(
wg *conc.WaitGroup, ctx context.Context, cancel context.CancelFunc, s service.Service,
) {
wg.Go(func() {
// Immediately acknowledge panicing services by shutting down the node
// Without the deffered cancel(), we would have to wait for user to hit Ctrl+C
defer cancel()
if err := s.Run(ctx); err != nil {
n.logger.Error(
"Service error",
zap.String("name", reflect.TypeOf(s).String()),
zap.Error(err),
)
}
})
}
The stated intent (per the inline comment) is to react to a panicking/failed service by tearing the whole node down. But the current implementation fires cancel() on any return, including a clean nil. That forces every service.Service implementation to keep Run blocking until ctx.Done() — even when it has nothing left to do — purely to avoid bringing the node down.
MigrationFeeder.Run (added in #3601) is an example of a service that has to artificially block on <-ctx.Done() for this reason:
https://github.com/NethermindEth/juno/blob/main/starknetdata/feeder/migration_feeder.go
Proposal
Only trigger the node-wide cancel when Run returns an error (or panics). A service that completes its work and returns nil should be allowed to exit without taking the rest of the node with it.
Roughly:
defer func() {
if r := recover(); r != nil {
// log + cancel
cancel()
panic(r) // or handle
}
}()
if err := s.Run(ctx); err != nil {
n.logger.Error("Service error", ...)
cancel()
}
Care needs to be taken around services that are expected to run for the node's lifetime — for those, an early clean return is itself surprising and may still warrant logging, but not necessarily a full shutdown.
Follow-up from #3601 (comment).
Problem
node.Node.StartServiceunconditionallydefer cancel()s the node-level context as soon as a service'sRunreturns:https://github.com/NethermindEth/juno/blob/main/node/node.go#L729-L744
The stated intent (per the inline comment) is to react to a panicking/failed service by tearing the whole node down. But the current implementation fires
cancel()on any return, including a cleannil. That forces everyservice.Serviceimplementation to keepRunblocking untilctx.Done()— even when it has nothing left to do — purely to avoid bringing the node down.MigrationFeeder.Run(added in #3601) is an example of a service that has to artificially block on<-ctx.Done()for this reason:https://github.com/NethermindEth/juno/blob/main/starknetdata/feeder/migration_feeder.go
Proposal
Only trigger the node-wide cancel when
Runreturns an error (or panics). A service that completes its work and returnsnilshould be allowed to exit without taking the rest of the node with it.Roughly:
Care needs to be taken around services that are expected to run for the node's lifetime — for those, an early clean return is itself surprising and may still warrant logging, but not necessarily a full shutdown.