From 1b032588999a9594112508804cfdbdadec01f36b Mon Sep 17 00:00:00 2001
From: Filippo Valsorda <hi@filippo.io>
Date: Fri, 2 Dec 2016 19:43:45 +0000
Subject: [PATCH] crypto/tls: simplify the Handshake locking

See https://groups.google.com/forum/#!topic/golang-dev/Xxiai-R_jH0

Change-Id: I6052695ece9aff9e3112c2fb176596fde8aa9cb2
---
 conn.go | 67 ++++++++++-----------------------------------------------
 1 file changed, 11 insertions(+), 56 deletions(-)

diff --git a/conn.go b/conn.go
index ac78161..9b601a5 100644
--- a/conn.go
+++ b/conn.go
@@ -33,16 +33,12 @@ type Conn struct {
 
 	// constant after handshake; protected by handshakeMutex
 	handshakeMutex sync.Mutex // handshakeMutex < in.Mutex, out.Mutex, errMutex
-	// handshakeCond, if not nil, indicates that a goroutine is committed
-	// to running the handshake for this Conn. Other goroutines that need
-	// to wait for the handshake can wait on this, under handshakeMutex.
-	handshakeCond *sync.Cond
-	handshakeErr  error   // error resulting from handshake
-	connID        []byte  // Random connection id
-	clientHello   []byte  // ClientHello packet contents
-	vers          uint16  // TLS version
-	haveVers      bool    // version has been negotiated
-	config        *Config // configuration passed to constructor
+	handshakeErr   error      // error resulting from handshake
+	connID         []byte     // Random connection id
+	clientHello    []byte     // ClientHello packet contents
+	vers           uint16     // TLS version
+	haveVers       bool       // version has been negotiated
+	config         *Config    // configuration passed to constructor
 	// handshakeComplete is true if the connection reached application data
 	// and it's equivalent to phase > handshakeRunning
 	handshakeComplete bool
@@ -1474,55 +1470,19 @@ func (c *Conn) closeNotify() error {
 // In TLS 1.3 Handshake returns after the client and server first flights,
 // without waiting for the Client Finished.
 func (c *Conn) Handshake() error {
-	// c.handshakeErr and c.phase == handshakeRunning are protected by
-	// c.handshakeMutex. In order to perform a handshake, we need to lock
-	// c.in also and c.handshakeMutex must be locked after c.in.
-	//
-	// However, if a Read() operation is hanging then it'll be holding the
-	// lock on c.in and so taking it here would cause all operations that
-	// need to check whether a handshake is pending (such as Write) to
-	// block.
-	//
-	// Thus we first take c.handshakeMutex to check whether a handshake is
-	// needed.
-	//
-	// If so then, previously, this code would unlock handshakeMutex and
-	// then lock c.in and handshakeMutex in the correct order to run the
-	// handshake. The problem was that it was possible for a Read to
-	// complete the handshake once handshakeMutex was unlocked and then
-	// keep c.in while waiting for network data. Thus a concurrent
-	// operation could be blocked on c.in.
-	//
-	// Thus handshakeCond is used to signal that a goroutine is committed
-	// to running the handshake and other goroutines can wait on it if they
-	// need. handshakeCond is protected by handshakeMutex.
 	c.handshakeMutex.Lock()
 	defer c.handshakeMutex.Unlock()
 
-	for {
-		if err := c.handshakeErr; err != nil {
-			return err
-		}
-		if c.handshakeComplete {
-			return nil
-		}
-		if c.handshakeCond == nil {
-			break
-		}
-
-		c.handshakeCond.Wait()
+	if err := c.handshakeErr; err != nil {
+		return err
+	}
+	if c.handshakeComplete {
+		return nil
 	}
-
-	// Set handshakeCond to indicate that this goroutine is committing to
-	// running the handshake.
-	c.handshakeCond = sync.NewCond(&c.handshakeMutex)
-	c.handshakeMutex.Unlock()
 
 	c.in.Lock()
 	defer c.in.Unlock()
 
-	c.handshakeMutex.Lock()
-
 	// The handshake cannot have completed when handshakeMutex was unlocked
 	// because this goroutine set handshakeCond.
 	if c.handshakeErr != nil || c.handshakeComplete {
@@ -1551,11 +1511,6 @@ func (c *Conn) Handshake() error {
 		panic("handshake should have had a result.")
 	}
 
-	// Wake any other goroutines that are waiting for this handshake to
-	// complete.
-	c.handshakeCond.Broadcast()
-	c.handshakeCond = nil
-
 	return c.handshakeErr
 }