Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add recvClose channel to stream #140

Merged
merged 1 commit into from May 9, 2023

Conversation

dmcgowan
Copy link
Member

Prevent panic from closing recv channel, which may be written to after close. Use a separate channel to signal recv has closed and check that channel on read and write.

Alternative to #139 for containerd/containerd#8390

@dmcgowan
Copy link
Member Author

Had to add additional

	select {
	case <-s.recvClose:
		return s.recvErr
	default:
	}

on receive to replace the err nil check. Since Go does not guarantee select order priority, messages could still be sent after a close was called, causing that test flake. If the receive and close happen at the same time, it isn't important which one wins.

Copy link
Member

@henry118 henry118 left a comment

Choose a reason for hiding this comment

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

LGTM

client.go Outdated

case <-cs.s.recvClose:
return cs.s.recvErr
case msg := <-cs.s.recv:
Copy link
Member

@Iceber Iceber May 4, 2023

Choose a reason for hiding this comment

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

Is it necessary to ensure that messages that have been sent to recv are processed?

var msg *streamMessage

select {
...
case <- cs.s.recvClose:
    select {
        case msg <- cs.s.recv:
        default:
            return cs.s.recvErr
    }
case msg <- cs.s.recv:
}
if msg.header.Type == messageTypeResponse {
...

Because there is a check before receiving data, s.recv will not always have data

	select {
	case <-s.recvClose:
		return s.recvErr
	default:
	}

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, good catch.
I think it is a good idea to drain/handle appropriately since this is a buffered channel and the client thinks it has been sent.
At worst we should have only 1 message to process, currently.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated, that is a good change. Wish select could have ordering which would make code much cleaner

Prevent panic from closing recv channel, which may be written to after
close. Use a separate channel to signal recv has closed and check that
channel on read and write.

Signed-off-by: Derek McGowan <derek@mcg.dev>
Copy link
Member

@cpuguy83 cpuguy83 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
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

@fuweid fuweid merged commit 0ca69a9 into containerd:main May 9, 2023
11 checks passed
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.

None yet

5 participants