-
Notifications
You must be signed in to change notification settings - Fork 257
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Done() <- chan struct{}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comment nit, lgtm overall
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.