Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
| return fmt.Errorf("peerTransport.dial(): %w", err) | ||
| } | ||
| s.SpawnBg(func() error { return tcpConn.Run(ctx) }) | ||
| s.SpawnBg(func() error { return utils.IgnoreCancel(tcpConn.Run(ctx)) }) |
There was a problem hiding this comment.
fyi, this one is intentional, because peer is not expected to end the connection first.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2940 +/- ##
===========================================
+ Coverage 57.76% 68.73% +10.97%
===========================================
Files 2111 35 -2076
Lines 174240 2930 -171310
===========================================
- Hits 100653 2014 -98639
+ Misses 64631 743 -63888
+ Partials 8956 173 -8783
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
|
||
| func (c Conn) Run(ctx context.Context) error { | ||
| return utils.IgnoreCancel(scope.Run(ctx, func(ctx context.Context, s scope.Scope) error { | ||
| return scope.Run(ctx, func(ctx context.Context, s scope.Scope) error { |
There was a problem hiding this comment.
Why are we removing this IgnoreCancel?
There was a problem hiding this comment.
to handle the error in a more robust way at the call site
| // For example - if you have a tcp connection, then during cleanup one end will disconnect faster than the other, | ||
| // causing a race condition between context cancellation and disconnection error. | ||
| func IgnoreAfterCancel(ctx context.Context, err error) error { | ||
| if ctx.Err() != nil { |
There was a problem hiding this comment.
// IgnoreAfterCancel silently drops the error if the context is already canceled.
Does it? It drops any old error in context. There could be different types of error here.
I am sorry I sound like a broken record: These utility functions are anti-pattern: they add more LoC than they save; and more cognitive load than it should take to read simple go code.
There was a problem hiding this comment.
Does it? It drops any old error in context. There could be different types of error here.
True, context may contain various errors as well, I use word "canceled" to not say "context is done" or "context.Done() channel is closed" because it sounds weird and I've never heard anyone saying this. Also given that context is propagated to arbitrary subroutines, allowing to terminate/cancell/close done channel of context with an error different Canceled was a design mistake, because it makes literally almost ALL of golang code need to handle arbitrary errors they are not even aware of.
These utility functions are anti-pattern: they add more LoC than they save; and more cognitive load than it should take to read simple go code.
Well, that's like your opinion, man. I find your select clauses hardly readable, if you really would like to know.
4 participants
Added more precise handling of background tasks in tests which are allowed to fail during test cleanup.