-
Notifications
You must be signed in to change notification settings - Fork 60
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
moduleService: Fix return of nil
from starting function when starting failed
#417
Conversation
8bc3685
to
1057381
Compare
modules/modules_test.go
Outdated
// Start service using context that's going to be cancelled | ||
require.NoError(t, moduleSvc.StartAsync(ctx)) | ||
// We get context cancelled error from service context cancellation. | ||
require.ErrorIs(t, moduleSvc.AwaitRunning(context.Background()), context.Canceled) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that original test used ctx
for waiting, but that wait got canceled in the test.
@@ -104,19 +103,14 @@ func (w *moduleService) run(serviceContext context.Context) error { | |||
|
|||
func (w *moduleService) stop(_ error) error { | |||
var err error | |||
switch w.service.State() { | |||
case services.Running: | |||
if w.service.State() == services.Running { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reverts to previous logic, as underlying service can't be in Starting
state when we get here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I guess @dimitarvdimitrov should review since he has the context (no pun intended).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I don't think deadlock is possible in this case. Thanks for proposing this. You can also edit the changelog entry to mention this PR
modules/modules_test.go
Outdated
// If moduleService can handle StartingStates, then it should call Stop on the subservice. | ||
require.NoError(t, services.StopAndAwaitTerminated(context.Background(), moduleSvc)) | ||
// We get context.Canceled from moduleService.start. | ||
require.ErrorIs(t, services.StopAndAwaitTerminated(context.Background(), moduleSvc), context.Canceled) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit confusing IMO that we get a context cancelation from an unrelated context. I think wrapping the context cancellation will be clearer. I made another suggestion
modules/module_service.go
Outdated
if err != nil { | ||
// Make sure that underlying service is stopped before returning | ||
// (eg. in case of context cancellation, AwaitRunning returns early, but service may still be starting). | ||
_ = services.StopAndAwaitTerminated(context.Background(), w.service) | ||
} | ||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return err | |
return errors.Wrapf(err, "waiting for service %s to start", w.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Makes sense. I was thinking the same when writing the test.
…ng failed (#417) * moduleService.start should not return nil if starting failed. * Wrap error returned when waiting for module service start. * Add mention in CHANGELOG. * Include module name in the check.
What this PR does:
This is alternative fix to #409, as discussed in thread #409 (comment).
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]