[thrift] branch master updated: THRIFT-5214: Reset read deadline in socketConn

[dc...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

dcelasun pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/thrift.git


The following commit(s) were added to refs/heads/master by this push:
     new cfbb905  THRIFT-5214: Reset read deadline in socketConn
cfbb905 is described below

commit cfbb905034c928f073639af00b30d74951744b61
Author: Yuxuan 'fishy' Wang <yu...@reddit.com>
AuthorDate: Tue Jun 9 13:07:38 2020 -0700

    THRIFT-5214: Reset read deadline in socketConn
    
    Client: go
    
    This is a slightly different, and less error-prone approach from the
    fix in e382275b.
    
    The previous approach relies on passing the set socket timeout into the
    underlying socketConn from TSocket and TSSLSocket. But since we have so
    many different constructors for TSocket and TSSLSocket, some makes the
    initial connection in the constructor and some does not, there are so
    many different places we would need to remember to pass socketTimeout
    into socketConn. In the future, when we add another constructor to them,
    we could either forget to pass the socket timeout into socketConn, or
    try to pass it while we haven't constructed socketConn yet (which will
    cause panic), both are bad.
    
    In this approach we just clear the read deadline in the connectivity
    check read. Because that's a non-blocking read, it would work just fine
    without a read deadline.
---
 lib/go/thrift/socket.go           | 11 ++++-------
 lib/go/thrift/socket_conn.go      | 10 ++++++----
 lib/go/thrift/socket_unix_conn.go | 15 +++++++++------
 lib/go/thrift/ssl_socket.go       | 12 ++++--------
 4 files changed, 23 insertions(+), 25 deletions(-)

diff --git a/lib/go/thrift/socket.go b/lib/go/thrift/socket.go
index 5080894..eb94faf 100644
--- a/lib/go/thrift/socket.go
+++ b/lib/go/thrift/socket.go
@@ -58,9 +58,7 @@ func NewTSocketFromAddrTimeout(addr net.Addr, connTimeout time.Duration, soTimeo
 
 // Creates a TSocket from an existing net.Conn
 func NewTSocketFromConnTimeout(conn net.Conn, connTimeout time.Duration) *TSocket {
-	sock := &TSocket{conn: wrapSocketConn(conn), addr: conn.RemoteAddr(), connectTimeout: connTimeout, socketTimeout: connTimeout}
-	sock.conn.socketTimeout = connTimeout
-	return sock
+	return &TSocket{conn: wrapSocketConn(conn), addr: conn.RemoteAddr(), connectTimeout: connTimeout, socketTimeout: connTimeout}
 }
 
 // Sets the connect timeout
@@ -72,9 +70,6 @@ func (p *TSocket) SetConnTimeout(timeout time.Duration) error {
 // Sets the socket timeout
 func (p *TSocket) SetSocketTimeout(timeout time.Duration) error {
 	p.socketTimeout = timeout
-	if p.conn != nil {
-		p.conn.socketTimeout = timeout
-	}
 	return nil
 }
 
@@ -114,7 +109,6 @@ func (p *TSocket) Open() error {
 	)); err != nil {
 		return NewTTransportException(NOT_OPEN, err.Error())
 	}
-	p.conn.socketTimeout = p.socketTimeout
 	return nil
 }
 
@@ -151,6 +145,9 @@ func (p *TSocket) Read(buf []byte) (int, error) {
 		return 0, NewTTransportException(NOT_OPEN, "Connection not open")
 	}
 	p.pushDeadline(true, false)
+	// NOTE: Calling any of p.IsOpen, p.conn.read0, or p.conn.IsOpen between
+	// p.pushDeadline and p.conn.Read could cause the deadline set inside
+	// p.pushDeadline being reset, thus need to be avoided.
 	n, err := p.conn.Read(buf)
 	return n, NewTTransportExceptionFromError(err)
 }
diff --git a/lib/go/thrift/socket_conn.go b/lib/go/thrift/socket_conn.go
index 5ed598e..40aea61 100644
--- a/lib/go/thrift/socket_conn.go
+++ b/lib/go/thrift/socket_conn.go
@@ -23,16 +23,14 @@ import (
 	"bytes"
 	"io"
 	"net"
-	"time"
 )
 
 // socketConn is a wrapped net.Conn that tries to do connectivity check.
 type socketConn struct {
 	net.Conn
 
-	socketTimeout time.Duration
-	buf           bytes.Buffer
-	buffer        [1]byte
+	buf    bytes.Buffer
+	buffer [1]byte
 }
 
 var _ net.Conn = (*socketConn)(nil)
@@ -78,6 +76,10 @@ func (sc *socketConn) isValid() bool {
 // connection is nil.
 //
 // Otherwise, it tries to do a connectivity check and returns the result.
+//
+// It also has the side effect of resetting the previously set read deadline on
+// the socket. As a result, it shouldn't be called between setting read deadline
+// and doing actual read.
 func (sc *socketConn) IsOpen() bool {
 	if !sc.isValid() {
 		return false
diff --git a/lib/go/thrift/socket_unix_conn.go b/lib/go/thrift/socket_unix_conn.go
index 789b4fa..4535e75 100644
--- a/lib/go/thrift/socket_unix_conn.go
+++ b/lib/go/thrift/socket_unix_conn.go
@@ -27,6 +27,11 @@ import (
 	"time"
 )
 
+// We rely on this variable to be the zero time,
+// but define it as global variable to avoid repetitive allocations.
+// Please DO NOT mutate this variable in any way.
+var zeroTime time.Time
+
 func (sc *socketConn) read0() error {
 	return sc.checkConn()
 }
@@ -38,12 +43,10 @@ func (sc *socketConn) checkConn() error {
 		return nil
 	}
 
-	// Push read deadline
-	var t time.Time
-	if sc.socketTimeout > 0 {
-		t = time.Now().Add(sc.socketTimeout)
-	}
-	sc.Conn.SetReadDeadline(t)
+	// The reading about to be done here is non-blocking so we don't really
+	// need a read deadline. We just need to clear the previously set read
+	// deadline, if any.
+	sc.Conn.SetReadDeadline(zeroTime)
 
 	rc, err := syscallConn.SyscallConn()
 	if err != nil {
diff --git a/lib/go/thrift/ssl_socket.go b/lib/go/thrift/ssl_socket.go
index 6e90438..31ef3a0 100644
--- a/lib/go/thrift/ssl_socket.go
+++ b/lib/go/thrift/ssl_socket.go
@@ -62,17 +62,12 @@ func NewTSSLSocketFromAddrTimeout(addr net.Addr, cfg *tls.Config, timeout time.D
 
 // Creates a TSSLSocket from an existing net.Conn
 func NewTSSLSocketFromConnTimeout(conn net.Conn, cfg *tls.Config, timeout time.Duration) *TSSLSocket {
-	sock := &TSSLSocket{conn: wrapSocketConn(conn), addr: conn.RemoteAddr(), timeout: timeout, cfg: cfg}
-	sock.conn.socketTimeout = timeout
-	return sock
+	return &TSSLSocket{conn: wrapSocketConn(conn), addr: conn.RemoteAddr(), timeout: timeout, cfg: cfg}
 }
 
 // Sets the socket timeout
 func (p *TSSLSocket) SetTimeout(timeout time.Duration) error {
 	p.timeout = timeout
-	if p.conn != nil {
-		p.conn.socketTimeout = timeout
-	}
 	return nil
 }
 
@@ -106,7 +101,6 @@ func (p *TSSLSocket) Open() error {
 		)); err != nil {
 			return NewTTransportException(NOT_OPEN, err.Error())
 		}
-		p.conn.socketTimeout = p.timeout
 	} else {
 		if p.conn.isValid() {
 			return NewTTransportException(ALREADY_OPEN, "Socket already connected.")
@@ -130,7 +124,6 @@ func (p *TSSLSocket) Open() error {
 		)); err != nil {
 			return NewTTransportException(NOT_OPEN, err.Error())
 		}
-		p.conn.socketTimeout = p.timeout
 	}
 	return nil
 }
@@ -163,6 +156,9 @@ func (p *TSSLSocket) Read(buf []byte) (int, error) {
 		return 0, NewTTransportException(NOT_OPEN, "Connection not open")
 	}
 	p.pushDeadline(true, false)
+	// NOTE: Calling any of p.IsOpen, p.conn.read0, or p.conn.IsOpen between
+	// p.pushDeadline and p.conn.Read could cause the deadline set inside
+	// p.pushDeadline being reset, thus need to be avoided.
 	n, err := p.conn.Read(buf)
 	return n, NewTTransportExceptionFromError(err)
 }