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

stream: fix the map of streams leak #120

Merged
merged 1 commit into from
Sep 29, 2022
Merged

Conversation

wllenyj
Copy link
Contributor

@wllenyj wllenyj commented Jun 6, 2022

In a connection, streams are added only, but not deleted.
When replying to the last data, delete the stream from map.

And delete operations require concurrency.

Signed-off-by: wllenyj wllenyj@linux.alibaba.com

@wllenyj wllenyj changed the title stream: fix the map of streams overflow stream: fix the map of streams leak Jun 6, 2022
In a connection, streams are added only, but not deleted.
When replying to the last data, delete the stream from map.

And delete operations require concurrency.

Signed-off-by: wllenyj <wllenyj@linux.alibaba.com>
@dmcgowan
Copy link
Member

dmcgowan commented Jun 8, 2022

Ahh yes, thanks

My original thought was to use a buffered channel to mark the removals to be handled in the receive go routine. The motivation being to avoid a locking data structure on the receive path. However, a flood of stream closing which fill up the buffer would likely make that solution unworkable. I'll try to consider other lock free ways we could handle this, but we should get this change in.

@wllenyj
Copy link
Contributor Author

wllenyj commented Jun 9, 2022

@dmcgowan thanks.
Streaming is a great project, the logic in it is so complex that it took me a lot of time to understand it, I admire your ability to come up with this stuff :)

Copy link
Member

@kzys kzys left a comment

Choose a reason for hiding this comment

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

Looks good to me.

How did you notice the issue by the way? It would be great if we can write some autometed way to check that this part won't have leaks in future.

@wllenyj
Copy link
Contributor Author

wllenyj commented Jul 13, 2022

How did you notice the issue by the way? It would be great if we can write some autometed way to check that this part won't have leaks in future.

I added the test log locally. If we want to cover it by unit test, we need to save streams to serverConn in order to check it at some point later. But it is expected that there will be more modifications.

@dmcgowan dmcgowan added this to the 1.7 milestone Jul 27, 2022
@dmcgowan dmcgowan merged commit 89444d6 into containerd:main Sep 29, 2022
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.

4 participants