Conversation
brandur
force-pushed
the
brandur-shutdown-context-cause
branch
2 times, most recently
from
fb41aa7 to
99a2378
Compare
|
|
||
| conn, err := n.establishConn(ctx) | ||
| if err != nil { | ||
| if errors.Is(err, context.Canceled) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hmm, hard to tell without digging into the code, but maybe it's possible pgx doesn't surface this source error correctly? establishConn is a simple method that just returns the result of pgx.ConnectConfig
There was a problem hiding this comment.
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.
…or logging Follow up to a discussion [1] in which an additional error log was causing example tests to fail after uses of `log` were replaced with `slog` and no longer suppressed. Here, use `WithCancelCause` to send a specific cancellation error on shutdown, which can be handled specially for instances where we'd only want to log an error under unusual circumstances. [1] #140 (comment)
brandur
force-pushed
the
brandur-shutdown-context-cause
branch
from
99a2378 to
1f236a6
Compare
|
@bgentry This one's a follow up from the conversation in #140 (comment) — roughly what you had in mind? I think this solves the logging problem that was failing example tests before. If I recall correctly, the failures we were seeing before the special patch in #140 would reproduce quite reliably. I reran CI a few times here to try and make a failure happen, but it seems to be fairly stable as written. |
| } | ||
|
|
||
| stopCtx, stopCancel := context.WithTimeout(ctx, 5*time.Second) | ||
| t.Cleanup(stopCancel) |
There was a problem hiding this comment.
I took this out because with the addition of require.ErrorIs above, 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.
bgentry
left a comment
bgentry
left a comment
There was a problem hiding this comment.
Thanks! I've been meaning to take a pass through some places like this and add specific cancellation causes where possible (ever since we started using those APIs).
|
|
||
| conn, err := n.establishConn(ctx) | ||
| if err != nil { | ||
| if errors.Is(err, context.Canceled) { |
There was a problem hiding this comment.
Hmm, hard to tell without digging into the code, but maybe it's possible pgx doesn't surface this source error correctly? establishConn is a simple method that just returns the result of pgx.ConnectConfig
|
Thanks! I'll skip a changelog entry on this one since as written, it's not a user-facing change. |
2 participants
Follow up to a discussion [1] in which an additional error log was
causing example tests to fail after uses of
logwere replaced withslogand no longer suppressed.Here, use
WithCancelCauseto send a specific cancellation error onshutdown, which can be handled specially for instances where we'd only
want to log an error under unusual circumstances.
[1] #140 (comment)