cmd/podman/utils.go: Cancel-able resize writes

On Thu, Jun 28, 2018 at 03:48:26AM -0700, Marco Vedovati wrote [1]:
> The root cause is a deadlock between two channel writes made by two
> different goroutines:
>
> 1. `resizeTty() : go func(){} : sendUpdate()` is sending a resize
>    message thru `resize` right at the beginning, but the channel is
>    never read if some startup error occurs.
>
> 2. Upon program termination, `startAttachCtr() : defer func(){} ` is
>    telling the goroutine in "1." to stop via the `resizeTerminate`
>    channel. But that guy is still waiting for the write to `resize`
>    to complete so the the termination message is never read.
>
> I think the go deadlock detection does not kick in because not all
> goroutines are seen as asleep. E.g. `os/signal Notify()` is enough
> to have the deadlock not detected.

333ab8c2 (Fix podman hangs when detecting startup error in container
attached mode, 2018-06-27, #1010) addressed this with a deferred
drain.  This commit adjusts that approach to use a single select to
cover "have we been canceled?", "has there been a resize signal?", and
(when we have one) "can we write the most recent resize event to the
resize channel?".

A side benefit to this approach is that if we have a slow resize
consumer and several resize signals, the resizeTty function will keep
updating its local resizeEvent.  Once the resize channel is able to
accept, only the most-recent event will be written.  Previously we'd
have written one resize event for every received signal, even if the
resize consumer was falling behind.

[1]: https://github.com/projectatomic/libpod/pull/1010#issuecomment-400994436

Signed-off-by: W. Trevor King <wking@tremily.us>

Closes: #1018
Approved by: rhatdan

1 files changed, 42 insertions, 57 deletions

diff --git a/cmd/podman/utils.go b/cmd/podman/utils.go
index 522b74926..25bb36b60 100644
--- a/cmd/podman/utils.go
+++ b/cmd/podman/utils.go
@@ -1,6 +1,7 @@
 package main
 
 import (
+	"context"
 	"fmt"
 	"os"
 	gosignal "os/signal"
@@ -19,6 +20,7 @@ type RawTtyFormatter struct {
 
 // Attach to a container
 func attachCtr(ctr *libpod.Container, stdout, stderr, stdin *os.File, detachKeys string, sigProxy bool) error {
+	ctx := context.Background()
 	resize := make(chan remotecommand.TerminalSize)
 
 	haveTerminal := terminal.IsTerminal(int(os.Stdin.Fd()))
@@ -28,12 +30,10 @@ func attachCtr(ctr *libpod.Container, stdout, stderr, stdin *os.File, detachKeys
 	if haveTerminal && ctr.Spec().Process.Terminal {
 		logrus.Debugf("Handling terminal attach")
 
-		resizeTerminate := make(chan interface{})
-		defer func() {
-			resizeTerminate <- true
-			close(resizeTerminate)
-		}()
-		resizeTty(resize, resizeTerminate)
+		subCtx, cancel := context.WithCancel(ctx)
+		defer cancel()
+
+		resizeTty(subCtx, resize)
 
 		oldTermState, err := term.SaveState(os.Stdin.Fd())
 		if err != nil {
@@ -46,19 +46,6 @@ func attachCtr(ctr *libpod.Container, stdout, stderr, stdin *os.File, detachKeys
 		defer restoreTerminal(oldTermState)
 	}
 
-	// There may be on the other size of the resize channel a goroutine trying to send.
-	// So consume all data inside the channel before exiting to avoid a deadlock.
-	defer func() {
-		for {
-			select {
-			case <-resize:
-				logrus.Debugf("Consumed resize command from channel")
-			default:
-				return
-			}
-		}
-	}()
-
 	streams := new(libpod.AttachStreams)
 	streams.OutputStream = stdout
 	streams.ErrorStream = stderr
@@ -89,6 +76,7 @@ func attachCtr(ctr *libpod.Container, stdout, stderr, stdin *os.File, detachKeys
 
 // Start and attach to a container
 func startAttachCtr(ctr *libpod.Container, stdout, stderr, stdin *os.File, detachKeys string, sigProxy bool) error {
+	ctx := context.Background()
 	resize := make(chan remotecommand.TerminalSize)
 
 	haveTerminal := terminal.IsTerminal(int(os.Stdin.Fd()))
@@ -98,12 +86,10 @@ func startAttachCtr(ctr *libpod.Container, stdout, stderr, stdin *os.File, detac
 	if haveTerminal && ctr.Spec().Process.Terminal {
 		logrus.Debugf("Handling terminal attach")
 
-		resizeTerminate := make(chan interface{})
-		defer func() {
-			resizeTerminate <- true
-			close(resizeTerminate)
-		}()
-		resizeTty(resize, resizeTerminate)
+		subCtx, cancel := context.WithCancel(ctx)
+		defer cancel()
+
+		resizeTty(subCtx, resize)
 
 		oldTermState, err := term.SaveState(os.Stdin.Fd())
 		if err != nil {
@@ -116,19 +102,6 @@ func startAttachCtr(ctr *libpod.Container, stdout, stderr, stdin *os.File, detac
 		defer restoreTerminal(oldTermState)
 	}
 
-	// There may be on the other size of the resize channel a goroutine trying to send.
-	// So consume all data inside the channel before exiting to avoid a deadlock.
-	defer func() {
-		for {
-			select {
-			case <-resize:
-				logrus.Debugf("Consumed resize command from channel")
-			default:
-				return
-			}
-		}
-	}()
-
 	streams := new(libpod.AttachStreams)
 	streams.OutputStream = stdout
 	streams.ErrorStream = stderr
@@ -171,33 +144,45 @@ func startAttachCtr(ctr *libpod.Container, stdout, stderr, stdin *os.File, detac
 	return nil
 }
 
+// getResize returns a TerminalSize command matching stdin's current
+// size on success, and nil on errors.
+func getResize() *remotecommand.TerminalSize {
+	winsize, err := term.GetWinsize(os.Stdin.Fd())
+	if err != nil {
+		logrus.Warnf("Could not get terminal size %v", err)
+		return nil
+	}
+	return &remotecommand.TerminalSize{
+		Width:  winsize.Width,
+		Height: winsize.Height,
+	}
+}
+
 // Helper for prepareAttach - set up a goroutine to generate terminal resize events
-func resizeTty(resize chan remotecommand.TerminalSize, resizeTerminate chan interface{}) {
+func resizeTty(ctx context.Context, resize chan remotecommand.TerminalSize) {
 	sigchan := make(chan os.Signal, 1)
 	gosignal.Notify(sigchan, signal.SIGWINCH)
-	sendUpdate := func() {
-		winsize, err := term.GetWinsize(os.Stdin.Fd())
-		if err != nil {
-			logrus.Warnf("Could not get terminal size %v", err)
-			return
-		}
-		resize <- remotecommand.TerminalSize{
-			Width:  winsize.Width,
-			Height: winsize.Height,
-		}
-	}
 	go func() {
 		defer close(resize)
 		// Update the terminal size immediately without waiting
 		// for a SIGWINCH to get the correct initial size.
-		sendUpdate()
+		resizeEvent := getResize()
 		for {
-			select {
-			case <-resizeTerminate:
-				return
-
-			case <-sigchan:
-				sendUpdate()
+			if resizeEvent == nil {
+				select {
+				case <-ctx.Done():
+					return
+				case <-sigchan:
+					resizeEvent = getResize()
+				}
+			} else {
+				select {
+				case <-ctx.Done():
+					return
+				case <-sigchan:
+					resizeEvent = getResize()
+				case resize <- *resizeEvent:
+				}
 			}
 		}
 	}()