-
Notifications
You must be signed in to change notification settings - Fork 138
Specify specific cancel cause on shutdown and use it to determine error logging #179
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ import ( | |
|
|
||
| "github.com/riverqueue/river/internal/baseservice" | ||
| "github.com/riverqueue/river/internal/componentstatus" | ||
| "github.com/riverqueue/river/internal/rivercommon" | ||
| ) | ||
|
|
||
| const statementTimeout = 5 * time.Second | ||
|
|
@@ -135,18 +136,7 @@ func (n *Notifier) getConnAndRun(ctx context.Context) { | |
|
|
||
| conn, err := n.establishConn(ctx) | ||
| if err != nil { | ||
| if errors.Is(err, context.Canceled) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One thing I wasn't quite able to figure out is why this condition wasn't detecting the cancellation previously. I would've thought that even without a special cancellation error, this still would've been enough to intercept a problem before falling through to logging.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, hard to tell without digging into the code, but maybe it's possible pgx doesn't surface this source error correctly?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yeah, that's probably it actually. The error being checked is now coming from a different place (context), which would explain why this version works. |
||
| return | ||
| } | ||
| // Log at a lower verbosity level in case an error is received when the | ||
| // context is already done (probably because the client is stopping). | ||
| // Example tests can finish before the notifier connects and starts | ||
| // listening, and on client stop may produce a connection error that | ||
| // would otherwise pollute output and fail the test. | ||
| select { | ||
| case <-ctx.Done(): | ||
| n.logger.Info("error establishing connection from pool", "err", err) | ||
| default: | ||
| if !errors.Is(context.Cause(ctx), rivercommon.ErrShutdown) { | ||
| n.logger.Error("error establishing connection from pool", "err", err) | ||
| } | ||
| return | ||
|
|
||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| package rivercommon | ||
|
|
||
| import "errors" | ||
|
|
||
| // These constants are made available in rivercommon so that they're accessible | ||
| // by internal packages, but the top-level river package re-exports them, and | ||
| // all user code must use that set instead. | ||
| const ( | ||
| MaxAttemptsDefault = 25 | ||
| PriorityDefault = 1 | ||
| QueueDefault = "default" | ||
| ) | ||
|
|
||
| // ErrShutdown is a special error injected by the client into its fetch and work | ||
| // CancelCauseFuncs when it's stopping. It may be used by components for such | ||
| // cases like avoiding logging an error during a normal shutdown procedure. This | ||
| // is internal for the time being, but we could also consider exposing it. | ||
| var ErrShutdown = errors.New("shutdown initiated") |
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.
I took this out because with the addition of
require.ErrorIsabove, it revealed that it occasionally causes an intermittent failure as it kicks in and supersedes the shutdown's cancellation. I don't think this is needed anyway because if shutdown's context cancellation doesn't succeed, I don't think we could expect this one to succeed either. Also, not having keeps the test more succinct.