Correctly handle reading from events channel#2309
Correctly handle reading from events channel#2309crosbymichael merged 1 commit intocontainerd:masterfrom
Conversation
|
FYI. This is a |
There was a problem hiding this comment.
This is guaranteed to never return nil for e?
There was a problem hiding this comment.
you need a separate check for e != nil. the bool just tells you if the channel is still open or not, it does not signal what the value returned is
Codecov Report
@@ Coverage Diff @@
## master #2309 +/- ##
=======================================
Coverage 45.54% 45.54%
=======================================
Files 83 83
Lines 9185 9185
=======================================
Hits 4183 4183
Misses 4326 4326
Partials 676 676
Continue to review full report at Codecov.
|
10419b5 to
77fa943
Compare
|
@crosbymichael I reported the bug because it was the easiest way to replicate it. I tried running a small code snippet with your changes but I get this func main() {
c, err := containerd.New(endpoint)
if err != nil {
return err
}
for {
eventsCh, errCh := c.EventService().Subscribe(ctx.Background(), `topic~="/containers/create"`)
err := listenEvents(eventsCh, errCh)
fmt.Printf("err: %s\n", err)
}
}
func listenEvents(eventsCh <-chan *events.Envelope, errCh <-chan error) error {
open := true
for open {
var e *events.Envelope
select {
case e, open = <-eventsCh:
case err := <-errCh:
return err
}
var out []byte
if e.Event != nil {
v, err := typeurl.UnmarshalAny(e.Event)
if err != nil {
return err
}
out, err = json.Marshal(v)
if err != nil {
return err
}
}
if _, err := fmt.Println(
e.Timestamp,
e.Namespace,
e.Topic,
string(out),
); err != nil {
return err
}
}
return nil
} |
|
@aanm i tried your snipped and it works for me. That error you are getting now is unrelated to this change and maybe a mismatch of versions on your system when you are compiling a 1.1 containerd and and older client, or vise versa, it's hard to tell. |
|
@crosbymichael I noticed I fixed this is a slightly different way when updating the code for moby https://github.com/moby/moby/pull/36895/files The |
|
@dmcgowan ok, i'll update it tomorrow morning |
|
LGTM. This is my mistake... |
Signed-off-by: Michael Crosby <[email protected]>
77fa943 to
0906879
Compare
|
@dmcgowan updated to handle close from the error chan. |
Fixes #2308
Channel handling was backwards, the bool represents more not closed.
Signed-off-by: Michael Crosby [email protected]