bugfix: return the context error during event subscribe #2742
Conversation
Signed-off-by: Wei Fu <[email protected]>
fuweid
force-pushed
the
bugfix_return_context_cancel_error_during_subcribe
branch
from
9aecb11 to
15a761c
Compare
Codecov Report
@@ Coverage Diff @@
## master #2742 +/- ##
==========================================
- Coverage 47.41% 43.74% -3.68%
==========================================
Files 91 100 +9
Lines 8410 10728 +2318
==========================================
+ Hits 3988 4693 +705
- Misses 3699 5305 +1606
- Partials 723 730 +7
Continue to review full report at Codecov.
|
|
LGTM |
| @@ -110,6 +110,9 @@ func (e *eventRemote) Subscribe(ctx context.Context, filters ...string) (ch <-ch | |||
| Event: ev.Event, | |||
| }: | |||
| case <-ctx.Done(): | |||
| if cerr := ctx.Err(); cerr != context.Canceled { | |||
There was a problem hiding this comment.
Needs to check the cause: errors.Cause(cerr) == context.Canceled).
There was a problem hiding this comment.
@stevvooe when the ctx.Done() is closed, the ctx.Err() will explaining why, canceled or deadline passed. It's no need to use pkg/errors or check nil here.
| @@ -110,6 +110,9 @@ func (e *eventRemote) Subscribe(ctx context.Context, filters ...string) (ch <-ch | |||
| Event: ev.Event, | |||
| }: | |||
| case <-ctx.Done(): | |||
| if cerr := ctx.Err(); cerr != context.Canceled { | |||
| errq <- cerr | |||
There was a problem hiding this comment.
Need to check against nil to avoid a write of a nil error. Should probably be in the condition above.
There was a problem hiding this comment.
+1 why aren't we checking for a nil error?
There was a problem hiding this comment.
@cpuguy83 according to the doc https://golang.org/pkg/context/#Context, when the ctx.Done() has been closed, the ctx.Err() always returns the non-nil to explain why it is done. maybe it's caused by canceled or deadline exceeded.
It depends on the implementation of context. Add nil check is good here if there is some bug in context. But for now, I think it's ok.
There was a problem hiding this comment.
Ok I see the full context of this now (haha).
|
It's hard to reproduce it and the https://github.com/containerd/containerd/blob/master/events/exchange/exchange.go#L193 does the same thing. just update it for the pr. |
|
I think we can trust Go to respect their interface for LGTM |
Signed-off-by: Wei Fu [email protected]