pkg/compose: composeService.Up: rewrite without go-multierror #13177
Conversation
| // if we get a second signal during shutdown, we kill the services | ||
| // immediately, so the channel needs to have sufficient capacity or | ||
| // we might miss a signal while setting up the second channel read | ||
| // (this is also why signal.Notify is used vs signal.NotifyContext) | ||
| signalChan := make(chan os.Signal, 2) | ||
| defer close(signalChan) | ||
| signal.Notify(signalChan, syscall.SIGINT, syscall.SIGTERM) | ||
| defer signal.Stop(signalChan) |
There was a problem hiding this comment.
Closing the signalChan isn't needed for these; signal.Stop should be enough.
| select { | ||
| case <-doneCh: | ||
| case <-globalCtx.Done(): | ||
| if watcher != nil { |
There was a problem hiding this comment.
I had a bit of input from my AI friends to see if we could have everything handled through the context (there were some paths that were a bit involved, and I saw some paths were possibly errgroup.Wait were not handled).
So pay close attention if I did it right, as I'm not so familiar with this code ❤️ 🙈
pkg/compose/up.go
Outdated
| _, attachCtx := errgroup.WithContext(ctx) | ||
| containers, err := s.attach(attachCtx, project, printer.HandleEvent, options.Start.AttachTo) | ||
| // | ||
| // FIXME(thaJeztah): this context was never cancelled, so didn't add anything? See https://github.com/docker/compose/commit/2c12ad19db5003cdf4cc0de8711970dbb6914b17 | ||
| // _, attachCtx := errgroup.WithContext(ctx) | ||
| // containers, err := s.attach(attachCtx, project, printer.HandleEvent, options.Start.AttachTo) | ||
| containers, err := s.attach(globalCtx, project, printer.HandleEvent, options.Start.AttachTo) |
There was a problem hiding this comment.
This part seemed like it was not yet functional, or at least, the derived context was never cancelled, and the new error-group was discarded, so effectively it was just a copy of the context that was never cancelled (added in 2c12ad1)
I didn't remove the code yet, because it probably needs to be looked at, and either removed, or perhaps the "attach" ones being in their own errgroup? I wasn't sure, so could use input here.
There was a problem hiding this comment.
seems to me this is legacy code that wasn't removed during recent refactoring
There was a problem hiding this comment.
OK, let me remove the comment and commented-out code
- Use a errgroup.Group and add a appendErr utility to not fail-fast, but collect errors. - replace doneCh for a global context to cancel goroutines - Commented out attachCtx code, as it didn't appear to be functional (as it wouldn't be cancelled). Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
rm_multierror_step2
branch
from
4a8f839 to
666906e
Compare
|
CI is green again; ptal @ndeloof 🤗 |
What I did
Related issue
(not mandatory) A picture of a cute animal, if possible in relation to what you did