Skip to content

stream: fix the map of streams leak#120

Merged
dmcgowan merged 1 commit intocontainerd:mainfrom
wllenyj:fix-streams
Sep 29, 2022
Merged

stream: fix the map of streams leak#120
dmcgowan merged 1 commit intocontainerd:mainfrom
wllenyj:fix-streams

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 [email protected]

@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 <[email protected]>
@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