Skip to content

Correctly handle reading from events channel#2309

Merged
crosbymichael merged 1 commit intocontainerd:masterfrom
crosbymichael:events-closed
Apr 25, 2018
Merged

Correctly handle reading from events channel#2309
crosbymichael merged 1 commit intocontainerd:masterfrom
crosbymichael:events-closed

Conversation

@crosbymichael
Copy link
Copy Markdown
Member

@crosbymichael crosbymichael commented Apr 24, 2018

Fixes #2308

Channel handling was backwards, the bool represents more not closed.

Signed-off-by: Michael Crosby [email protected]

@crosbymichael
Copy link
Copy Markdown
Member Author

FYI. This is a ctr only bug in 1.1. I don't think anything else is affected.

Comment thread cmd/ctr/commands/events/events.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is guaranteed to never return nil for e?

Copy link
Copy Markdown
Member Author

@crosbymichael crosbymichael Apr 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added this check

@codecov-io
Copy link
Copy Markdown

codecov-io commented Apr 24, 2018

Codecov Report

Merging #2309 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2309   +/-   ##
=======================================
  Coverage   45.54%   45.54%           
=======================================
  Files          83       83           
  Lines        9185     9185           
=======================================
  Hits         4183     4183           
  Misses       4326     4326           
  Partials      676      676
Flag Coverage Δ
#linux 50.07% <ø> (ø) ⬆️
#windows 41.24% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 209a7fc...0906879. Read the comment docs.

@aanm
Copy link
Copy Markdown

aanm commented Apr 24, 2018

@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

err: type with url containerd.events.ContainerCreate: not found
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
}

@crosbymichael
Copy link
Copy Markdown
Member Author

@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.

@dmcgowan
Copy link
Copy Markdown
Member

@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 Subscribe function never actually closes the event channel, just the err channel. In that case I am not just ignoring a nil event and going back to the select, waiting for the error channel to signal completion.

@crosbymichael
Copy link
Copy Markdown
Member Author

@dmcgowan ok, i'll update it tomorrow morning

@Random-Liu
Copy link
Copy Markdown
Member

LGTM. This is my mistake...

@crosbymichael
Copy link
Copy Markdown
Member Author

@dmcgowan updated to handle close from the error chan.

Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Copy Markdown
Member

@dmcgowan dmcgowan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@crosbymichael crosbymichael merged commit e073a48 into containerd:master Apr 25, 2018
@crosbymichael crosbymichael deleted the events-closed branch April 25, 2018 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ctr events is broken in 1.1.0

6 participants