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.
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 @@
// 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 @@
// 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 @@
)); err != nil {
return NewTTransportException(NOT_OPEN, err.Error())
}
- p.conn.socketTimeout = p.socketTimeout
return nil
}
@@ -151,6 +145,9 @@
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 @@
"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 @@
// 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 @@
"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 @@
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 @@
// 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 @@
)); 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 @@
)); err != nil {
return NewTTransportException(NOT_OPEN, err.Error())
}
- p.conn.socketTimeout = p.timeout
}
return nil
}
@@ -163,6 +156,9 @@
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)
}