From 1d5ff12b612d8007557e818c53a0499c742deab8 Mon Sep 17 00:00:00 2001 From: Randy Reddig Date: Mon, 18 Oct 2021 15:41:14 -0700 Subject: [PATCH 01/12] NewConn now just calls NewTimeoutConn(..., 0) --- conn.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/conn.go b/conn.go index 4dd722c..9d7e995 100644 --- a/conn.go +++ b/conn.go @@ -43,12 +43,7 @@ type Conn struct { // handshake. It reads and stores the initial EPP message. // https://tools.ietf.org/html/rfc5730#section-2.4 func NewConn(conn net.Conn) (*Conn, error) { - c := newConn(conn) - g, err := c.readGreeting() - if err == nil { - c.Greeting = g - } - return c, err + return NewTimeoutConn(conn, 0) } // NewTimeoutConn initializes an epp.Conn like NewConn, limiting the duration of network From be92541da47b7ca6fbf098ff282ddf66cfb69358 Mon Sep 17 00:00:00 2001 From: Randy Reddig Date: Tue, 19 Oct 2021 14:31:35 -0700 Subject: [PATCH 02/12] Serialize reads and writes to the underlying connection This allows concurrent use of the epp.Conn. The next step is to support clTRID (client transaction identifiers) to enable command pipelining. --- check.go | 10 ++- conn.go | 179 ++++++++++++++++++++++++++++++++--------------- conn_test.go | 45 ++---------- greeting.go | 11 +-- greeting_test.go | 5 +- info.go | 5 +- session.go | 14 ++-- 7 files changed, 145 insertions(+), 124 deletions(-) diff --git a/check.go b/check.go index a5a7249..26c9bee 100644 --- a/check.go +++ b/check.go @@ -23,13 +23,12 @@ func (c *Conn) CheckDomainExtensions(domains []string, extData map[string]string return nil, err } - err = c.writeDataUnit(x) + err = c.writeRequest(x) if err != nil { return nil, err } - var res Response - err = c.readResponse(&res) + res, err := c.readResponse() if err != nil { return nil, err } @@ -41,12 +40,11 @@ func (c *Conn) CheckDomainExtensions(domains []string, extData map[string]string if err != nil { return nil, err } - err = c.writeDataUnit(x) + err = c.writeRequest(x) if err != nil { return nil, err } - var res2 Response - err = c.readResponse(&res2) + res2, err := c.readResponse() if err != nil { return nil, err } diff --git a/conn.go b/conn.go index 9d7e995..480be7e 100644 --- a/conn.go +++ b/conn.go @@ -6,6 +6,7 @@ import ( "encoding/xml" "io" "net" + "sync" "time" ) @@ -19,24 +20,38 @@ func IgnoreEOF(err error) error { } // Conn represents a single connection to an EPP server. -// This implementation is not safe for concurrent use. +// Reads and writes are serialized, so it is safe for concurrent use. type Conn struct { + // Conn is the underlying net.Conn (usually a TLS connection). net.Conn - buf bytes.Buffer - decoder *xml.Decoder - saved xml.Decoder + + // Timeout defines the timeout for network operations. + // It must be set at initialization. Changing it after + // a connection is already opened will have no effect. + Timeout time.Duration + + // m protects Greeting and LoginResult. + m sync.Mutex // Greeting holds the last received greeting message from the server, // indicating server name, status, data policy and capabilities. + // + // Deprecated: This field is written to upon opening a new EPP connection and should not be modified. Greeting // LoginResult holds the last received login response message's Result // from the server, in which some servers might include diagnostics such // as connection count limits. + // + // Deprecated: this field is written to by the Login method but otherwise is not used by this package. LoginResult Result - // Timeout defines the timeout for network operations. - Timeout time.Duration + requests chan []byte + responses chan *Response + + done chan struct{} + readErr error + writeErr error } // NewConn initializes an epp.Conn from a net.Conn and performs the EPP @@ -60,72 +75,125 @@ func NewTimeoutConn(conn net.Conn, timeout time.Duration) (*Conn, error) { // Close sends an EPP command and closes the connection c. func (c *Conn) Close() error { + select { + case <-c.done: + return net.ErrClosed + default: + } c.Logout() + close(c.done) return c.Conn.Close() } // newConn initializes an epp.Conn from a net.Conn. // Used internally for testing. func newConn(conn net.Conn) *Conn { - c := Conn{Conn: conn} - c.decoder = xml.NewDecoder(&c.buf) - c.saved = *c.decoder + c := Conn{ + Conn: conn, + requests: make(chan []byte), + responses: make(chan *Response), + done: make(chan struct{}), + } + go c.writeLoop() + go c.readLoop() return &c } -// reset resets the underlying xml.Decoder and bytes.Buffer, -// restoring the original state of the underlying -// xml.Decoder (pos 1, line 1, stack, etc.) using a hack. -func (c *Conn) reset() { - c.buf.Reset() - *c.decoder = c.saved // Heh. +// writeRequest queues a single EPP request (x) for writing on c. +// It returns net.ErrClosed if the underlying connection is closed. +// writeRequest can be called from multiple goroutines. +func (c *Conn) writeRequest(x []byte) error { + select { + case c.requests <- x: + return nil + case <-c.done: + return net.ErrClosed + } +} + +// readResponse dequeues and returns a EPP response from c. +// It returns an error if the EPP response contains an error Result. +// readResponse can be called from multiple goroutines. +func (c *Conn) readResponse() (*Response, error) { + select { + case res := <-c.responses: + if res == nil { + return res, c.readErr + } + if res.Result.IsError() { + return nil, &res.Result + } + return res, nil + case <-c.done: + return nil, net.ErrClosed + } +} + +func (c *Conn) writeLoop() { + defer c.Close() + for { + // TODO(ydnar): figure out how to handle timeouts for continous write loop. + select { + case x := <-c.requests: + err := writeDataUnit(c.Conn, x) + if err != nil { + c.writeErr = err + return + } + case <-c.done: + return + } + } +} + +func (c *Conn) readLoop() { + defer close(c.responses) + buf := &bytes.Buffer{} + decoder := xml.NewDecoder(buf) + saved := *decoder + for { + // TODO(ydnar): figure out how to handle timeouts for continous read loop. + // if c.Timeout > 0 { + // c.Conn.SetReadDeadline(time.Now().Add(c.Timeout)) + // } + err := readDataUnit(buf, c.Conn) + if err != nil { + c.readErr = err + return + } + res := &Response{} + *decoder = saved + err = IgnoreEOF(scanResponse.Scan(decoder, res)) + if err != nil { + c.readErr = err + return + } + c.responses <- res + } } -// writeDataUnit writes a slice of bytes to c. +// writeDataUnit writes x to w. // Bytes written are prefixed with 32-bit header specifying the total size // of the data unit (message + 4 byte header), in network (big-endian) order. // http://www.ietf.org/rfc/rfc4934.txt -func (c *Conn) writeDataUnit(x []byte) error { +func writeDataUnit(w io.Writer, x []byte) error { logXML("<-- WRITE DATA UNIT -->", x) s := uint32(4 + len(x)) - if c.Timeout > 0 { - c.Conn.SetWriteDeadline(time.Now().Add(c.Timeout)) - } - err := binary.Write(c.Conn, binary.BigEndian, s) + err := binary.Write(w, binary.BigEndian, s) if err != nil { return err } - _, err = c.Conn.Write(x) + _, err = w.Write(x) return err } -// readResponse reads a single EPP response from c and parses the XML into req. -// It returns an error if the EPP response contains an error Result. -func (c *Conn) readResponse(res *Response) error { - err := c.readDataUnit() - if err != nil { - return err - } - err = IgnoreEOF(scanResponse.Scan(c.decoder, res)) - if err != nil { - return err - } - if res.Result.IsError() { - return &res.Result - } - return nil -} - -// readDataUnit reads a single EPP message from c into -// c.buf. The bytes in c.buf are valid until the next -// call to readDataUnit. -func (c *Conn) readDataUnit() error { - c.reset() +// readDataUnit reads a single EPP data unit from r, returning the payload bytes or an error. +// An EPP data units is prefixed with 32-bit header specifying the total size +// of the data unit (message + 4 byte header), in network (big-endian) order. +// http://www.ietf.org/rfc/rfc4934.txt +func readDataUnit(buf *bytes.Buffer, r io.Reader) error { var s int32 - if c.Timeout > 0 { - c.Conn.SetReadDeadline(time.Now().Add(c.Timeout)) - } - err := binary.Read(c.Conn, binary.BigEndian, &s) + err := binary.Read(r, binary.BigEndian, &s) if err != nil { return err } @@ -133,16 +201,11 @@ func (c *Conn) readDataUnit() error { if s < 0 { return io.ErrUnexpectedEOF } - lr := io.LimitedReader{R: c.Conn, N: int64(s)} - n, err := c.buf.ReadFrom(&lr) - if err != nil { - return err - } - if n != int64(s) || lr.N != 0 { - return io.ErrUnexpectedEOF - } - logXML("<-- READ DATA UNIT -->", c.buf.Bytes()) - return nil + buf.Reset() + buf.Grow(int(s)) + _, err = io.CopyN(buf, r, int64(s)) + logXML("<-- READ DATA UNIT -->", buf.Bytes()) + return err } func deleteRange(s, pfx, sfx []byte) []byte { diff --git a/conn_test.go b/conn_test.go index 26fbb19..cfd92b3 100644 --- a/conn_test.go +++ b/conn_test.go @@ -1,7 +1,7 @@ package epp import ( - "encoding/xml" + "bytes" "net" "sync" "testing" @@ -34,7 +34,7 @@ func (ls *localServer) teardown() { } func newLocalServer() (*localServer, error) { - ln, err := net.Listen("tcp", ":0") + ln, err := net.Listen("tcp", "127.0.0.1:0") if err != nil { return nil, err } @@ -48,13 +48,12 @@ func TestNewConn(t *testing.T) { ls.buildup(func(ls *localServer, ln net.Listener) { conn, err := ls.Accept() st.Assert(t, err, nil) - sc := newConn(conn) // Respond with greeting - err = sc.writeDataUnit([]byte(testXMLGreeting)) + err = writeDataUnit(conn, []byte(testXMLGreeting)) st.Assert(t, err, nil) - var res Response // Read logout message - err = sc.readResponse(&res) + buf := &bytes.Buffer{} + err = readDataUnit(buf, conn) st.Assert(t, err, nil) // Close connection err = conn.Close() @@ -62,7 +61,6 @@ func TestNewConn(t *testing.T) { }) nc, err := net.Dial(ls.Listener.Addr().Network(), ls.Listener.Addr().String()) st.Assert(t, err, nil) - c, err := NewConn(nc) st.Assert(t, err, nil) st.Reject(t, c, nil) @@ -71,39 +69,6 @@ func TestNewConn(t *testing.T) { st.Expect(t, err, nil) } -func TestConnDecoderReuse(t *testing.T) { - c := newConn(nil) - v := struct { - XMLName struct{} `xml:"hello"` - Foo string `xml:"foo"` - }{} - - c.reset() - c.buf.WriteString(`foo`) - st.Expect(t, c.decoder.InputOffset(), int64(0)) - c.decoder.Decode(&v) - st.Expect(t, v.Foo, "foo") - st.Expect(t, c.decoder.InputOffset(), int64(29)) - - c.reset() - c.buf.WriteString(`bar`) - st.Expect(t, c.decoder.InputOffset(), int64(0)) - tok, _ := c.decoder.Token() - se := tok.(xml.StartElement) - st.Expect(t, se.Name.Local, "hello") - tok, _ = c.decoder.Token() - se = tok.(xml.StartElement) - st.Expect(t, se.Name.Local, "foo") - st.Expect(t, c.decoder.InputOffset(), int64(12)) - - c.reset() - c.buf.WriteString(`blam<`) - st.Expect(t, c.decoder.InputOffset(), int64(0)) - c.decoder.Decode(&v) - st.Expect(t, v.Foo, "blam<") - st.Expect(t, c.decoder.InputOffset(), int64(34)) -} - func TestDeleteRange(t *testing.T) { v := deleteRange([]byte(``), []byte(``)) st.Expect(t, string(v), ``) diff --git a/greeting.go b/greeting.go index 3ddbcf9..78d93ea 100644 --- a/greeting.go +++ b/greeting.go @@ -8,7 +8,7 @@ import ( // Hello sends a command to request a from the EPP server. func (c *Conn) Hello() error { - err := c.writeDataUnit(xmlHello) + err := c.writeRequest(xmlHello) if err != nil { return err } @@ -102,14 +102,9 @@ var ExtURNNames = map[string]string{ "neulevel-1.0": ExtNeulevel10, } +// TODO: check if res.Greeting is not empty. func (c *Conn) readGreeting() (Greeting, error) { - err := c.readDataUnit() - if err != nil { - return Greeting{}, err - } - deleteBufferRange(&c.buf, []byte(``), []byte(``)) - var res Response - err = IgnoreEOF(scanResponse.Scan(c.decoder, &res)) + res, err := c.readResponse() if err != nil { return Greeting{}, err } diff --git a/greeting_test.go b/greeting_test.go index 79f7b46..dbbcfc4 100644 --- a/greeting_test.go +++ b/greeting_test.go @@ -16,12 +16,11 @@ func TestHello(t *testing.T) { ls.buildup(func(ls *localServer, ln net.Listener) { conn, err := ls.Accept() st.Assert(t, err, nil) - sc := newConn(conn) // Respond with greeting - err = sc.writeDataUnit([]byte(testXMLGreeting)) + err = writeDataUnit(conn, []byte(testXMLGreeting)) st.Assert(t, err, nil) // Respond with greeting for - err = sc.writeDataUnit([]byte(testXMLGreeting)) + err = writeDataUnit(conn, []byte(testXMLGreeting)) st.Assert(t, err, nil) }) nc, err := net.Dial(ls.Listener.Addr().Network(), ls.Listener.Addr().String()) diff --git a/info.go b/info.go index 5f6c173..d732cfd 100644 --- a/info.go +++ b/info.go @@ -15,12 +15,11 @@ func (c *Conn) DomainInfo(domain string, extData map[string]string) (*DomainInfo if err != nil { return nil, err } - err = c.writeDataUnit(x) + err = c.writeRequest(x) if err != nil { return nil, err } - var res Response - err = c.readResponse(&res) + res, err := c.readResponse() if err != nil { return nil, err } diff --git a/session.go b/session.go index 1982d27..d738476 100644 --- a/session.go +++ b/session.go @@ -12,8 +12,10 @@ func (c *Conn) Login(user, password, newPassword string) error { if err != nil { return err } - var res Response - err = c.readResponse(&res) + res, err := c.readResponse() + if err != nil { + return nil + } // We always have a .Result in our non-pointer, but it might be meaningless. // We might not have read anything. We think that the worst case is we // have the same zero values we'd get without the assignment-even-in-error-case. @@ -33,7 +35,7 @@ func (c *Conn) writeLogin(user, password, newPassword string) error { if err != nil { return err } - return c.writeDataUnit(x) + return c.writeRequest(x) } func encodeLogin(user, password, newPassword, version, language string, objects, extensions []string) ([]byte, error) { @@ -75,12 +77,12 @@ func encodeLogin(user, password, newPassword, version, language string, objects, // Logout sends a command to terminate an EPP session. // https://tools.ietf.org/html/rfc5730#section-2.9.1.2 func (c *Conn) Logout() error { - err := c.writeDataUnit(xmlLogout) + err := c.writeRequest(xmlLogout) if err != nil { return err } - var res Response - return c.readResponse(&res) + _, err = c.readResponse() + return err } var xmlLogout = []byte(xmlCommandPrefix + `` + xmlCommandSuffix) From cd63b84471dd6c3ba79d1c6071be762cde7f21ca Mon Sep 17 00:00:00 2001 From: Randy Reddig Date: Tue, 19 Oct 2021 17:11:00 -0700 Subject: [PATCH 03/12] Add mutex around Conn.Greeting and .LoginResult TODO: break backwards compatibility by unexporting these. --- conn.go | 2 ++ session.go | 2 ++ 2 files changed, 4 insertions(+) diff --git a/conn.go b/conn.go index 480be7e..e9449a0 100644 --- a/conn.go +++ b/conn.go @@ -68,7 +68,9 @@ func NewTimeoutConn(conn net.Conn, timeout time.Duration) (*Conn, error) { c.Timeout = timeout g, err := c.readGreeting() if err == nil { + c.m.Lock() c.Greeting = g + c.m.Unlock() } return c, err } diff --git a/session.go b/session.go index d738476..7c5f4cd 100644 --- a/session.go +++ b/session.go @@ -19,7 +19,9 @@ func (c *Conn) Login(user, password, newPassword string) error { // We always have a .Result in our non-pointer, but it might be meaningless. // We might not have read anything. We think that the worst case is we // have the same zero values we'd get without the assignment-even-in-error-case. + c.m.Lock() c.LoginResult = res.Result + c.m.Unlock() return err } From 831ffaf42643d30c493de53243203ebcd655df41 Mon Sep 17 00:00:00 2001 From: Randy Reddig Date: Wed, 17 Nov 2021 14:29:08 -0800 Subject: [PATCH 04/12] remove writeLoop goroutine; use mutex --- conn.go | 40 +++++++++------------------------------- 1 file changed, 9 insertions(+), 31 deletions(-) diff --git a/conn.go b/conn.go index e9449a0..cfbb022 100644 --- a/conn.go +++ b/conn.go @@ -46,12 +46,13 @@ type Conn struct { // Deprecated: this field is written to by the Login method but otherwise is not used by this package. LoginResult Result - requests chan []byte + // mWrite synchronizes connection writes. + mWrite sync.Mutex + responses chan *Response + readErr error - done chan struct{} - readErr error - writeErr error + done chan struct{} } // NewConn initializes an epp.Conn from a net.Conn and performs the EPP @@ -92,25 +93,19 @@ func (c *Conn) Close() error { func newConn(conn net.Conn) *Conn { c := Conn{ Conn: conn, - requests: make(chan []byte), responses: make(chan *Response), done: make(chan struct{}), } - go c.writeLoop() go c.readLoop() return &c } -// writeRequest queues a single EPP request (x) for writing on c. -// It returns net.ErrClosed if the underlying connection is closed. +// writeRequest writes a single EPP request (x) for writing on c. // writeRequest can be called from multiple goroutines. func (c *Conn) writeRequest(x []byte) error { - select { - case c.requests <- x: - return nil - case <-c.done: - return net.ErrClosed - } + c.mWrite.Lock() + defer c.mWrite.Unlock() + return writeDataUnit(c.Conn, x) } // readResponse dequeues and returns a EPP response from c. @@ -131,23 +126,6 @@ func (c *Conn) readResponse() (*Response, error) { } } -func (c *Conn) writeLoop() { - defer c.Close() - for { - // TODO(ydnar): figure out how to handle timeouts for continous write loop. - select { - case x := <-c.requests: - err := writeDataUnit(c.Conn, x) - if err != nil { - c.writeErr = err - return - } - case <-c.done: - return - } - } -} - func (c *Conn) readLoop() { defer close(c.responses) buf := &bytes.Buffer{} From 78c0d3939dff52ef22dd53e80818569e6774e615 Mon Sep 17 00:00:00 2001 From: Randy Reddig Date: Thu, 18 Nov 2021 09:17:35 -0800 Subject: [PATCH 05/12] remove hack to reset xml.Decoder by using an io.LimitedReader --- conn.go | 31 +++++++++++++------------------ conn_test.go | 4 +--- 2 files changed, 14 insertions(+), 21 deletions(-) diff --git a/conn.go b/conn.go index cfbb022..46a67df 100644 --- a/conn.go +++ b/conn.go @@ -128,21 +128,20 @@ func (c *Conn) readResponse() (*Response, error) { func (c *Conn) readLoop() { defer close(c.responses) - buf := &bytes.Buffer{} - decoder := xml.NewDecoder(buf) - saved := *decoder + r := &io.LimitedReader{R: c.Conn} + decoder := xml.NewDecoder(r) for { // TODO(ydnar): figure out how to handle timeouts for continous read loop. // if c.Timeout > 0 { // c.Conn.SetReadDeadline(time.Now().Add(c.Timeout)) // } - err := readDataUnit(buf, c.Conn) + n, err := parseDataUnit(c.Conn) if err != nil { c.readErr = err return } + r.N = int64(n) res := &Response{} - *decoder = saved err = IgnoreEOF(scanResponse.Scan(decoder, res)) if err != nil { c.readErr = err @@ -167,25 +166,21 @@ func writeDataUnit(w io.Writer, x []byte) error { return err } -// readDataUnit reads a single EPP data unit from r, returning the payload bytes or an error. +// parseDataUnit reads a single EPP data unit header from r, returning the payload size or an error. // An EPP data units is prefixed with 32-bit header specifying the total size // of the data unit (message + 4 byte header), in network (big-endian) order. // http://www.ietf.org/rfc/rfc4934.txt -func readDataUnit(buf *bytes.Buffer, r io.Reader) error { - var s int32 - err := binary.Read(r, binary.BigEndian, &s) +func parseDataUnit(r io.Reader) (int32, error) { + var n int32 + err := binary.Read(r, binary.BigEndian, &n) if err != nil { - return err + return 0, err } - s -= 4 // https://tools.ietf.org/html/rfc5734#section-4 - if s < 0 { - return io.ErrUnexpectedEOF + n -= 4 // https://tools.ietf.org/html/rfc5734#section-4 + if n < 0 { + return 0, io.ErrUnexpectedEOF } - buf.Reset() - buf.Grow(int(s)) - _, err = io.CopyN(buf, r, int64(s)) - logXML("<-- READ DATA UNIT -->", buf.Bytes()) - return err + return n, err } func deleteRange(s, pfx, sfx []byte) []byte { diff --git a/conn_test.go b/conn_test.go index cfd92b3..2d3745f 100644 --- a/conn_test.go +++ b/conn_test.go @@ -1,7 +1,6 @@ package epp import ( - "bytes" "net" "sync" "testing" @@ -52,8 +51,7 @@ func TestNewConn(t *testing.T) { err = writeDataUnit(conn, []byte(testXMLGreeting)) st.Assert(t, err, nil) // Read logout message - buf := &bytes.Buffer{} - err = readDataUnit(buf, conn) + _, err = parseDataUnit(conn) st.Assert(t, err, nil) // Close connection err = conn.Close() From b596f354e1eb67783dd3a1299e3f0d253fdef537 Mon Sep 17 00:00:00 2001 From: Randy Reddig Date: Thu, 18 Nov 2021 09:20:42 -0800 Subject: [PATCH 06/12] move deleteRange and deleteBufferRange to tests --- conn.go | 21 --------------------- conn_test.go | 21 +++++++++++++++++++++ 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/conn.go b/conn.go index 46a67df..a12b1d9 100644 --- a/conn.go +++ b/conn.go @@ -1,7 +1,6 @@ package epp import ( - "bytes" "encoding/binary" "encoding/xml" "io" @@ -182,23 +181,3 @@ func parseDataUnit(r io.Reader) (int32, error) { } return n, err } - -func deleteRange(s, pfx, sfx []byte) []byte { - start := bytes.Index(s, pfx) - if start < 0 { - return s - } - end := bytes.Index(s[start+len(pfx):], sfx) - if end < 0 { - return s - } - end += start + len(pfx) + len(sfx) - size := len(s) - (end - start) - copy(s[start:size], s[end:]) - return s[:size] -} - -func deleteBufferRange(buf *bytes.Buffer, pfx, sfx []byte) { - v := deleteRange(buf.Bytes(), pfx, sfx) - buf.Truncate(len(v)) -} diff --git a/conn_test.go b/conn_test.go index 2d3745f..031d29a 100644 --- a/conn_test.go +++ b/conn_test.go @@ -1,6 +1,7 @@ package epp import ( + "bytes" "net" "sync" "testing" @@ -74,3 +75,23 @@ func TestDeleteRange(t *testing.T) { v = deleteRange([]byte(``), []byte(``), []byte(`o>`)) st.Expect(t, string(v), ``) } + +func deleteBufferRange(buf *bytes.Buffer, pfx, sfx []byte) { + v := deleteRange(buf.Bytes(), pfx, sfx) + buf.Truncate(len(v)) +} + +func deleteRange(s, pfx, sfx []byte) []byte { + start := bytes.Index(s, pfx) + if start < 0 { + return s + } + end := bytes.Index(s[start+len(pfx):], sfx) + if end < 0 { + return s + } + end += start + len(pfx) + len(sfx) + size := len(s) - (end - start) + copy(s[start:size], s[end:]) + return s[:size] +} From 2ab9c89a60c839c41b0b676ea0b5603640467d39 Mon Sep 17 00:00:00 2001 From: Randy Reddig Date: Thu, 18 Nov 2021 10:09:09 -0800 Subject: [PATCH 07/12] re-add timeouts --- conn.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/conn.go b/conn.go index a12b1d9..5341572 100644 --- a/conn.go +++ b/conn.go @@ -104,6 +104,9 @@ func newConn(conn net.Conn) *Conn { func (c *Conn) writeRequest(x []byte) error { c.mWrite.Lock() defer c.mWrite.Unlock() + if c.Timeout > 0 { + c.Conn.SetWriteDeadline(time.Now().Add(c.Timeout)) + } return writeDataUnit(c.Conn, x) } @@ -127,13 +130,13 @@ func (c *Conn) readResponse() (*Response, error) { func (c *Conn) readLoop() { defer close(c.responses) + timeout := c.Timeout r := &io.LimitedReader{R: c.Conn} decoder := xml.NewDecoder(r) for { - // TODO(ydnar): figure out how to handle timeouts for continous read loop. - // if c.Timeout > 0 { - // c.Conn.SetReadDeadline(time.Now().Add(c.Timeout)) - // } + if timeout > 0 { + c.Conn.SetReadDeadline(time.Now().Add(timeout)) + } n, err := parseDataUnit(c.Conn) if err != nil { c.readErr = err From eec67f9e243bb1921aa636c3744063391f27c004 Mon Sep 17 00:00:00 2001 From: Randy Reddig Date: Thu, 18 Nov 2021 11:58:27 -0800 Subject: [PATCH 08/12] PR feedback: fix comment --- conn.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/conn.go b/conn.go index 5341572..fa4eb94 100644 --- a/conn.go +++ b/conn.go @@ -169,7 +169,7 @@ func writeDataUnit(w io.Writer, x []byte) error { } // parseDataUnit reads a single EPP data unit header from r, returning the payload size or an error. -// An EPP data units is prefixed with 32-bit header specifying the total size +// An EPP data unit is prefixed with 32-bit header specifying the total size // of the data unit (message + 4 byte header), in network (big-endian) order. // http://www.ietf.org/rfc/rfc4934.txt func parseDataUnit(r io.Reader) (int32, error) { From fc1190d8f94ac03f42b477d5cd80bd6cb6ada4b1 Mon Sep 17 00:00:00 2001 From: Randy Reddig Date: Thu, 18 Nov 2021 11:58:41 -0800 Subject: [PATCH 09/12] int32->uint32 (PR feedback) --- conn.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/conn.go b/conn.go index fa4eb94..25cda91 100644 --- a/conn.go +++ b/conn.go @@ -172,8 +172,8 @@ func writeDataUnit(w io.Writer, x []byte) error { // An EPP data unit is prefixed with 32-bit header specifying the total size // of the data unit (message + 4 byte header), in network (big-endian) order. // http://www.ietf.org/rfc/rfc4934.txt -func parseDataUnit(r io.Reader) (int32, error) { - var n int32 +func parseDataUnit(r io.Reader) (uint32, error) { + var n uint32 err := binary.Read(r, binary.BigEndian, &n) if err != nil { return 0, err From 74edbaea1bf3d976a48d135f0c1cd4806a29be20 Mon Sep 17 00:00:00 2001 From: Randy Reddig Date: Thu, 18 Nov 2021 12:02:59 -0800 Subject: [PATCH 10/12] remove potential overflow bug --- conn.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/conn.go b/conn.go index 25cda91..c315035 100644 --- a/conn.go +++ b/conn.go @@ -178,9 +178,9 @@ func parseDataUnit(r io.Reader) (uint32, error) { if err != nil { return 0, err } - n -= 4 // https://tools.ietf.org/html/rfc5734#section-4 - if n < 0 { + if n < 4 { return 0, io.ErrUnexpectedEOF } - return n, err + // https://tools.ietf.org/html/rfc5734#section-4 + return n - 4, err } From 56816c95eab715c04cd9bfe4896a74aa5b024c89 Mon Sep 17 00:00:00 2001 From: Randy Reddig Date: Thu, 18 Nov 2021 12:37:37 -0800 Subject: [PATCH 11/12] parseDataUnit -> readDataUnitHeader --- conn.go | 6 +++--- conn_test.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/conn.go b/conn.go index c315035..4b82a68 100644 --- a/conn.go +++ b/conn.go @@ -137,7 +137,7 @@ func (c *Conn) readLoop() { if timeout > 0 { c.Conn.SetReadDeadline(time.Now().Add(timeout)) } - n, err := parseDataUnit(c.Conn) + n, err := readDataUnitHeader(c.Conn) if err != nil { c.readErr = err return @@ -168,11 +168,11 @@ func writeDataUnit(w io.Writer, x []byte) error { return err } -// parseDataUnit reads a single EPP data unit header from r, returning the payload size or an error. +// readDataUnitHeader reads a single EPP data unit header from r, returning the payload size or an error. // An EPP data unit is prefixed with 32-bit header specifying the total size // of the data unit (message + 4 byte header), in network (big-endian) order. // http://www.ietf.org/rfc/rfc4934.txt -func parseDataUnit(r io.Reader) (uint32, error) { +func readDataUnitHeader(r io.Reader) (uint32, error) { var n uint32 err := binary.Read(r, binary.BigEndian, &n) if err != nil { diff --git a/conn_test.go b/conn_test.go index 031d29a..c008277 100644 --- a/conn_test.go +++ b/conn_test.go @@ -52,7 +52,7 @@ func TestNewConn(t *testing.T) { err = writeDataUnit(conn, []byte(testXMLGreeting)) st.Assert(t, err, nil) // Read logout message - _, err = parseDataUnit(conn) + _, err = readDataUnitHeader(conn) st.Assert(t, err, nil) // Close connection err = conn.Close() From 12cceb39c0f269a488056f91c23e9ec08b01b09a Mon Sep 17 00:00:00 2001 From: Randy Reddig Date: Thu, 18 Nov 2021 12:47:22 -0800 Subject: [PATCH 12/12] remove newConn; fix data race with c.Timeout --- conn.go | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/conn.go b/conn.go index 4b82a68..520e62e 100644 --- a/conn.go +++ b/conn.go @@ -64,8 +64,13 @@ func NewConn(conn net.Conn) (*Conn, error) { // NewTimeoutConn initializes an epp.Conn like NewConn, limiting the duration of network // operations on conn using Set(Read|Write)Deadline. func NewTimeoutConn(conn net.Conn, timeout time.Duration) (*Conn, error) { - c := newConn(conn) - c.Timeout = timeout + c := &Conn{ + Conn: conn, + Timeout: timeout, + responses: make(chan *Response), + done: make(chan struct{}), + } + go c.readLoop() g, err := c.readGreeting() if err == nil { c.m.Lock() @@ -87,18 +92,6 @@ func (c *Conn) Close() error { return c.Conn.Close() } -// newConn initializes an epp.Conn from a net.Conn. -// Used internally for testing. -func newConn(conn net.Conn) *Conn { - c := Conn{ - Conn: conn, - responses: make(chan *Response), - done: make(chan struct{}), - } - go c.readLoop() - return &c -} - // writeRequest writes a single EPP request (x) for writing on c. // writeRequest can be called from multiple goroutines. func (c *Conn) writeRequest(x []byte) error {