Remove blocking on container exit for every new exec created #1601
Conversation
| @@ -90,4 +90,7 @@ type Container interface { | |||
| Wait() error | |||
| // Modify sends a request to modify container resources | |||
| Modify(ctx context.Context, config interface{}) error | |||
|
|
|||
| // WaitChannel returns the wait channel of the container | |||
| WaitChannel() <-chan struct{} | |||
There was a problem hiding this comment.
nit: Done() <- chan struct{}
There was a problem hiding this comment.
Since we are using waitBlock channel and waitError in the container structures, would WaitChannel() and WaitError() be better for readability?
There was a problem hiding this comment.
I am with Maksim on this, since Done mirrors how contexts do it and makes the convention match
internal/hcs/system.go
Outdated
| @@ -291,6 +291,10 @@ func (computeSystem *System) waitBackground() { | |||
| oc.SetSpanStatus(span, err) | |||
| } | |||
|
|
|||
| func (c *System) WaitChannel() <-chan struct{} { | |||
| return c.waitBlock | |||
There was a problem hiding this comment.
looking at the code we don't currently close c.closeCh anymore during Close()... I wonder what happened there.
There was a problem hiding this comment.
Yes! Infact I do not see closeCh being used anywhere too. Discussion on that here - https://github.com/microsoft/hcsshim/pull/1601/files#r1044855223
There was a problem hiding this comment.
We could cleanup and refactor in the later long term fix I guess
|
By the way, think this is fine to undraft. |
Commit fixes the memory leak seen in the shim. It removes creation of channel that waits on container exit for every new exec. Instead, the container wait channel is exposed through WaitChannel() function which callers can use to decide if container has exited or not. Signed-off-by: Kirtana Ashok <Kirtana.Ashok@microsoft.com>
|
By the way, was there a reason you merged |
No it was a mistake. I rebased and submitted to the PR soon after :) |
kiashok
changed the title
Fixes the memory leak seen in the shim. Removes the creation of channel that waits on container exit for every new exec created.
Instead, a new function 'WaitChannel()' is added to the Container interface to expose the wait channel of a container. The caller of WaitChannel() can use the container's wait channel to decide if the container has exited or not rather than creating a new channel for every exec and blocking on container.Wait() call.