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.

Sign up for GitHub

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

Jump to bottom

Fix regression for timed-out stream cleanups #102489

Merged
merged 1 commit into from Jun 4, 2021
Merged

Fix regression for timed-out stream cleanups #102489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/connection.go
View file
Open in desktop
Expand Up @@ -123,7 +123,10 @@ func (c *connection) Close() error {
func (c *connection) RemoveStreams(streams ...httpstream.Stream) {
c.streamLock.Lock()
for _, stream := range streams {
delete(c.streams, stream.Identifier())
// It may be possible that the provided stream is nil if timed out.
if stream != nil {
delete(c.streams, stream.Identifier())
}
Comment on lines +126 to +129
Copy link
Member

Choose a reason for hiding this comment

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

I feel that adding a judgment on the caller is also a good choice

kubernetes/pkg/kubelet/cri/streaming/portforward/httpstream.go

Line 168 in eee6e54

h.conn.RemoveStreams(pair.dataStream, pair.errorStream)

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @wzshiming, thank you for the review! Do you mean that we should move the nil check over to the RemoveStreams() invocation in favor of checking here?

Copy link
Member

Choose a reason for hiding this comment

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

it's fine to check in both places, but I want this check to stay here, since it prevents all mistakes.

Copy link

Choose a reason for hiding this comment

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

Question: will the timed out stream not remain hanging around in the c.streams map, since the identity is no longer known when calling RemoveStreams?

I guess it gets cleaned up during Close, but since the unit test below does an explicit len(c.streams) check, just wondering if something else might use a similiar check to determine whether the connection can be closed?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that's an excellent question, good reason to do reverts rather than another round of cherry-picks.

Copy link
Member Author

Choose a reason for hiding this comment

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

Tested it locally with a modified version of CRI-O (using this vendored code) and it looks like that the connections are getting cleaned up if the timeout got reached.

}
c.streamLock.Unlock()
}
Expand Down
3 changes: 3 additions & 0 deletions staging/src/k8s.io/apimachinery/pkg/util/httpstream/spdy/connection_test.go
View file
Open in desktop
Expand Up @@ -323,6 +323,9 @@ func TestConnectionRemoveStreams(t *testing.T) {
// remove all existing
c.RemoveStreams(stream0, stream1)

// remove nil stream should not crash
c.RemoveStreams(nil)
Copy link
Member

Choose a reason for hiding this comment

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

This is fine, but I'd be more confident with a test that reproduces the actual crash, that way we can be confident in the diagnosis and not just the fix.


if len(c.streams) != 0 {
t.Fatalf("should not have any streams, has %d", len(c.streams))
}
Expand Down