Skip to content
This repository has been archived by the owner on Mar 9, 2022. It is now read-only.

Commit

Permalink
Merge pull request #1418 from darfux/fix_handle_resizing_leak
Browse files Browse the repository at this point in the history
Fix goroutine leak when exec/attach
  • Loading branch information
AkihiroSuda committed Mar 28, 2020
2 parents 6756681 + cb01400 commit dd3c5f0
Show file tree
Hide file tree
Showing 3 changed files with 17 additions and 11 deletions.
4 changes: 3 additions & 1 deletion pkg/server/container_attach.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ func (c *criService) Attach(ctx context.Context, r *runtime.AttachRequest) (*run

func (c *criService) attachContainer(ctx context.Context, id string, stdin io.Reader, stdout, stderr io.WriteCloser,
tty bool, resize <-chan remotecommand.TerminalSize) error {
ctx, cancel := context.WithCancel(ctx)
defer cancel()
// Get container from our container store.
cntr, err := c.containerStore.Get(id)
if err != nil {
Expand All @@ -60,7 +62,7 @@ func (c *criService) attachContainer(ctx context.Context, id string, stdin io.Re
if err != nil {
return errors.Wrap(err, "failed to load task")
}
handleResizing(resize, func(size remotecommand.TerminalSize) {
handleResizing(ctx, resize, func(size remotecommand.TerminalSize) {
if err := task.Resize(ctx, uint32(size.Width), uint32(size.Height)); err != nil {
log.G(ctx).WithError(err).Errorf("Failed to resize task %q console", id)
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/server/container_execsync.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ func (c *criService) execInternal(ctx context.Context, container containerd.Cont
return nil, errors.Wrapf(err, "failed to start exec %q", execID)
}

handleResizing(opts.resize, func(size remotecommand.TerminalSize) {
handleResizing(ctx, opts.resize, func(size remotecommand.TerminalSize) {
if err := process.Resize(ctx, uint32(size.Width), uint32(size.Height)); err != nil {
log.G(ctx).WithError(err).Errorf("Failed to resize process %q console for container %q", execID, id)
}
Expand Down
22 changes: 13 additions & 9 deletions pkg/server/streaming.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ limitations under the License.
package server

import (
"context"
"crypto/tls"
"io"
"math"
Expand Down Expand Up @@ -160,9 +161,8 @@ func (s *streamRuntime) PortForward(podSandboxID string, port int32, stream io.R
}

// handleResizing spawns a goroutine that processes the resize channel, calling resizeFunc for each
// remotecommand.TerminalSize received from the channel. The resize channel must be closed elsewhere to stop the
// goroutine.
func handleResizing(resize <-chan remotecommand.TerminalSize, resizeFunc func(size remotecommand.TerminalSize)) {
// remotecommand.TerminalSize received from the channel.
func handleResizing(ctx context.Context, resize <-chan remotecommand.TerminalSize, resizeFunc func(size remotecommand.TerminalSize)) {
if resize == nil {
return
}
Expand All @@ -171,14 +171,18 @@ func handleResizing(resize <-chan remotecommand.TerminalSize, resizeFunc func(si
defer runtime.HandleCrash()

for {
size, ok := <-resize
if !ok {
select {
case <-ctx.Done():
return
case size, ok := <-resize:
if !ok {
return
}
if size.Height < 1 || size.Width < 1 {
continue
}
resizeFunc(size)
}
if size.Height < 1 || size.Width < 1 {
continue
}
resizeFunc(size)
}
}()
}
Expand Down

0 comments on commit dd3c5f0

Please sign in to comment.