go/neo/neonet: Rework handshake to differentiate client and server parts

Previously we were doing handshake symmetrically: both client and server
were transmitting hello and receiving peer's hello simultaneously.
However this does not allow server to adjust its behaviour depending on
which client (protocol version, protocol encoding, ...) is connecting to it.

-> Rework handshake so that client always sends its hello first, and
only then the server side replies. This matches actual NEO/py behaviour:

https://lab.nexedi.com/nexedi/neoppod/blob/v1.12-67-g261dd4b4/neo/lib/connector.py#L293-294

even though the "NEO protocol" states that

	Handshake transmissions are not ordered with respect to each other and can go in parallel.

	( https://neo.nexedi.com/P-NEO-Protocol.Specification.2019?portal_skin=CI_slideshow#/9/2 )

If I recall correctly that sentence was authored by me in 2018 based on
previous understanding of should-be full symmetry in-between client and
server.

However soon we are going to teach server sides to autodetect client
encoding and adjust server to talk to client via its preferred way.
This needs handshake for client and server to be differentiated.

The protocol needs to be adjusted as well. However I'm not sure it is
going to happen...

go/internal/xcontext/xcontext.go

+// Copyright (C) 2017-2021  Nexedi SA and Contributors.
+//                          Kirill Smelkov <kirr@nexedi.com>
+//
+// This program is free software: you can Use, Study, Modify and Redistribute
+// it under the terms of the GNU General Public License version 3, or (at your
+// option) any later version, as published by the Free Software Foundation.
+//
+// You can also Link and Combine this program with other software covered by
+// the terms of any of the Free Software licenses or any of the Open Source
+// Initiative approved licenses and Convey the resulting work. Corresponding
+// source of such a combination shall include the source code for all other
+// software used.
+//
+// This program is distributed WITHOUT ANY WARRANTY; without even the implied
+// warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+//
+// See COPYING file for full licensing terms.
+// See https://www.nexedi.com/licensing for rationale and options.
+
+// Package xcontext provides addons to std package context.
+package xcontext
+
+import (
+	"context"
+	"io"
+)
+
+
+// WithCloseOnErrCancel closes c on ctx cancel while f is run, or if f returns with an error.
+//
+// It is usually handy to propagate cancellation to interrupt IO.
+func WithCloseOnErrCancel(ctx context.Context, c io.Closer, f func() error) (err error) {
+	closed := false
+	fdone := make(chan error)
+	defer func() {
+		<-fdone // wait for f to complete
+
+		if err != nil {
+			if !closed {
+				c.Close()
+			}
+		}
+	}()
+
+	go func() (err error) {
+		defer func() {
+			fdone <- err
+			close(fdone)
+		}()
+		return f()
+	}()
+
+	select {
+	case <-ctx.Done():
+		c.Close() // interrupt IO
+		closed = true
+		return ctx.Err()
+
+	case err := <-fdone:
+		return err
+	}
+}

go/neo/neonet/connection.go

@@ -234,8 +234,6 @@ type ConnError struct {
 }
 
 // _LinkRole is a role an end of NodeLink is intended to play.
-//
-// XXX _LinkRole will need to become public again if _Handshake does.
 type _LinkRole int
 const (
 	_LinkServer _LinkRole = iota // link created as server

go/neo/neonet/connection_test.go

-// Copyright (C) 2016-2020  Nexedi SA and Contributors.
+// Copyright (C) 2016-2021  Nexedi SA and Contributors.
 //                          Kirill Smelkov <kirr@nexedi.com>
 //
 // This program is free software: you can Use, Study, Modify and Redistribute
@@ -1084,12 +1084,12 @@ func xlinkPipe(c1, c2 net.Conn) (*NodeLink, *NodeLink) {
 
 	wg := xsync.NewWorkGroup(context.Background())
 	gox(wg, func(ctx context.Context) {
-		l, err := _Handshake(ctx, c1, _LinkClient)
+		l, err := _HandshakeClient(ctx, c1)
 		exc.Raiseif(err)
 		l1 = l
 	})
 	gox(wg, func(ctx context.Context) {
-		l, err := _Handshake(ctx, c2, _LinkServer)
+		l, err := _HandshakeServer(ctx, c2)
 		exc.Raiseif(err)
 		l2 = l
 	})

go/neo/neonet/newlink.go

@@ -26,113 +26,181 @@ import (
 	"fmt"
 	"io"
 	"net"
-	"sync"
 
+	"lab.nexedi.com/kirr/go123/xerr"
 	"lab.nexedi.com/kirr/go123/xnet"
+	"lab.nexedi.com/kirr/neo/go/internal/xcontext"
 	"lab.nexedi.com/kirr/neo/go/internal/xio"
 	"lab.nexedi.com/kirr/neo/go/neo/proto"
 )
 
 // ---- Handshake ----
 
-// XXX _Handshake may be needed to become public in case when we have already
+// XXX _Handshake{Client,Server} may be needed to become public in case when we have already
 // established raw connection and want to hand-over it to NEO. But currently we
 // do not have such uses.
 
-// _Handshake performs NEO protocol handshake just after raw connection between
-// 2 nodes was established.
-//
-// On success raw connection is returned wrapped into NodeLink.
-// On error raw connection is closed.
-func _Handshake(ctx context.Context, conn net.Conn, role _LinkRole) (nl *NodeLink, err error) {
-	err = handshake(ctx, conn, proto.Version)
-	if err != nil {
-		return nil, err
-	}
+func _HandshakeClient(ctx context.Context, conn net.Conn) (*NodeLink, error) {
+	return handshakeClient(ctx, conn, proto.Version)
+}
 
-	// handshake ok -> NodeLink
-	return newNodeLink(conn, role), nil
+func _HandshakeServer(ctx context.Context, conn net.Conn) (*NodeLink, error) {
+	return handshakeServer(ctx, conn, proto.Version)
 }
 
 // _HandshakeError is returned when there is an error while performing handshake.
 type _HandshakeError struct {
+	LocalRole  _LinkRole
 	LocalAddr  net.Addr
 	RemoteAddr net.Addr
 	Err        error
 }
 
 func (e *_HandshakeError) Error() string {
-	return fmt.Sprintf("%s - %s: handshake: %s", e.LocalAddr, e.RemoteAddr, e.Err.Error())
+	role := ""
+	switch e.LocalRole {
+	case _LinkServer: role = "server"
+	case _LinkClient: role = "client"
+	default:          panic("bug")
+	}
+	return fmt.Sprintf("%s - %s: handshake (%s): %s", e.LocalAddr, e.RemoteAddr, role, e.Err.Error())
 }
 
 func (e *_HandshakeError) Cause()  error { return e.Err }
 func (e *_HandshakeError) Unwrap() error { return e.Err }
 
-func handshake(ctx context.Context, conn net.Conn, version uint32) (err error) {
-	// XXX simplify -> errgroup
-	errch := make(chan error, 2)
-
-	// tx handshake word
-	txWg := sync.WaitGroup{}
-	txWg.Add(1)
-	go func() {
-		var b [4]byte
-		binary.BigEndian.PutUint32(b[:], version) // XXX -> hton32 ?
-		_, err := conn.Write(b[:])
-		// XXX EOF -> ErrUnexpectedEOF ?
-		errch <- err
-		txWg.Done()
-	}()
+// handshakeClient implements client-side NEO protocol handshake just after raw
+// connection between 2 nodes was established.
+//
+// Client indicates its version to server.
+//
+// On success raw connection is returned wrapped into NodeLink.
+// On error raw connection is closed.
+func handshakeClient(ctx context.Context, conn net.Conn, version uint32) (*NodeLink, error) {
+	err := _handshakeClient(ctx, conn, version)
+	if err != nil {
+		return nil, err
+	}
+	return newNodeLink(conn, _LinkClient), nil
+}
 
-	// rx handshake word
-	go func() {
-		var b [4]byte
-		_, err := io.ReadFull(conn, b[:])
-		err = xio.NoEOF(err) // can be returned with n = 0
-		if err == nil {
-			peerVersion := binary.BigEndian.Uint32(b[:]) // XXX -> ntoh32 ?
-			if peerVersion != version {
-				err = fmt.Errorf("protocol version mismatch: peer = %08x  ; our side = %08x", peerVersion, version)
-			}
+// handshakeServer implements server-side NEO protocol handshake just after raw
+// connection between 2 nodes was established.
+//
+// Server verifies that its version matches Client.
+//
+// On success raw connection is returned wrapped into NodeLink.
+// On error raw connection is closed.
+func handshakeServer(ctx context.Context, conn net.Conn, version uint32) (*NodeLink, error) {
+	err := _handshakeServer(ctx, conn, version)
+	if err != nil {
+		return nil, err
+	}
+	return newNodeLink(conn, _LinkServer), nil
+}
+
+func _handshakeClient(ctx context.Context, conn net.Conn, version uint32) (err error) {
+	defer func() {
+		if err != nil {
+			err = &_HandshakeError{_LinkClient, conn.LocalAddr(), conn.RemoteAddr(), err}
 		}
-		errch <- err
 	}()
 
-	connClosed := false
-	defer func() {
-		// make sure our version is always sent on the wire, if possible,
-		// so that peer does not see just closed connection when on rx we see version mismatch.
-		//
-		// NOTE if cancelled tx goroutine will wake up without delay.
-		txWg.Wait()
+	err = xcontext.WithCloseOnErrCancel(ctx, conn, func() error {
+		// tx client hello
+		err := txHello("tx hello", conn, version)
+		if err != nil {
+			return err
+		}
 
-		// don't forget to close conn if returning with error + add handshake err context
+		// rx server hello reply
+		var peerVer uint32
+		peerVer, err = rxHello("rx hello reply", conn)
 		if err != nil {
-			err = &_HandshakeError{conn.LocalAddr(), conn.RemoteAddr(), err}
-			if !connClosed {
-				conn.Close()
-			}
+			return err
+		}
+
+		// verify version
+		if peerVer != version {
+			return fmt.Errorf("protocol version mismatch: peer = %08x  ; our side = %08x", peerVer, version)
+		}
+
+		return nil
+	})
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func _handshakeServer(ctx context.Context, conn net.Conn, version uint32) (err error) {
+	defer func() {
+		if err != nil {
+			err = &_HandshakeError{_LinkServer, conn.LocalAddr(), conn.RemoteAddr(), err}
 		}
 	}()
 
-	for i := 0; i < 2; i++ {
-		select {
-		case <-ctx.Done():
-			conn.Close() // interrupt IO
-			connClosed = true
-			return ctx.Err()
-
-		case err = <-errch:
-			if err != nil {
-				return err
-			}
+	err = xcontext.WithCloseOnErrCancel(ctx, conn, func() error {
+		// rx client hello
+		var peerVer uint32
+		var err     error
+		peerVer, err = rxHello("rx hello", conn)
+		if err != nil {
+			return err
+		}
+
+		// tx server reply
+		//
+		// do it before version check so that client can also detect "version
+		// mismatch" instead of just getting "disconnect".
+		err = txHello("tx hello reply", conn, version)
+		if err != nil {
+			return err
+		}
+
+		// verify version
+		if peerVer != version {
+			return fmt.Errorf("protocol version mismatch: peer = %08x  ; our side = %08x", peerVer, version)
 		}
+
+		return nil
+	})
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func txHello(errctx string, conn net.Conn, version uint32) (err error) {
+	defer xerr.Context(&err, errctx)
+
+	var b [4]byte
+	binary.BigEndian.PutUint32(b[:], version) // XXX -> hton32 ?
+
+	_, err = conn.Write(b[:])
+	if err != nil {
+		return err
 	}
 
-	// handshaked ok
 	return nil
 }
 
+func rxHello(errctx string, conn net.Conn) (version uint32, err error) {
+	defer xerr.Context(&err, errctx)
+
+	var b [4]byte
+	_, err = io.ReadFull(conn, b[:])
+	err = xio.NoEOF(err)
+	if err != nil {
+		return 0, err
+	}
+
+	peerVer := binary.BigEndian.Uint32(b[:]) // XXX -> ntoh32 ?
+	return peerVer, nil
+}
+
 
 // ---- Dial & Listen at NodeLink level ----
 
@@ -144,7 +212,7 @@ func DialLink(ctx context.Context, net xnet.Networker, addr string) (*NodeLink,
 		return nil, err
 	}
 
-	return _Handshake(ctx, peerConn, _LinkClient)
+	return _HandshakeClient(ctx, peerConn)
 }
 
 // ListenLink starts listening on laddr for incoming connections and wraps them as NodeLink.
@@ -192,7 +260,7 @@ func (l *linkListener) Accept(ctx context.Context) (*NodeLink, error) {
 	}
 
 	// NOTE Handshake closes conn in case of failure
-	link, err := _Handshake(ctx, conn, _LinkServer)
+	link, err := _HandshakeServer(ctx, conn)
 	if err != nil {
 		return nil, err
 	}

go/neo/neonet/newlink_test.go

-// Copyright (C) 2016-2020  Nexedi SA and Contributors.
+// Copyright (C) 2016-2021  Nexedi SA and Contributors.
 //                          Kirill Smelkov <kirr@nexedi.com>
 //
 // This program is free software: you can Use, Study, Modify and Redistribute
@@ -21,6 +21,7 @@ package neonet
 
 import (
 	"context"
+	"errors"
 	"io"
 	"net"
 	"testing"
@@ -29,8 +30,15 @@ import (
 	"lab.nexedi.com/kirr/go123/xsync"
 )
 
-func xhandshake(ctx context.Context, c net.Conn, version uint32) {
-	err := handshake(ctx, c, version)
+// _xhandshakeClient handshakes as client.
+func _xhandshakeClient(ctx context.Context, c net.Conn, version uint32) {
+	err := _handshakeClient(ctx, c, version)
+	exc.Raiseif(err)
+}
+
+// _xhandshakeServer handshakes as server.
+func _xhandshakeServer(ctx context.Context, c net.Conn, version uint32) {
+	err := _handshakeServer(ctx, c, version)
 	exc.Raiseif(err)
 }
 
@@ -40,10 +48,10 @@ func TestHandshake(t *testing.T) {
 	p1, p2 := net.Pipe()
 	wg := xsync.NewWorkGroup(bg)
 	gox(wg, func(ctx context.Context) {
-		xhandshake(ctx, p1, 1)
+		_xhandshakeClient(ctx, p1, 1)
 	})
 	gox(wg, func(ctx context.Context) {
-		xhandshake(ctx, p2, 1)
+		_xhandshakeServer(ctx, p2, 1)
 	})
 	xwait(wg)
 	xclose(p1)
@@ -54,17 +62,17 @@ func TestHandshake(t *testing.T) {
 	var err1, err2 error
 	wg = xsync.NewWorkGroup(bg)
 	gox(wg, func(ctx context.Context) {
-		err1 = handshake(ctx, p1, 1)
+		err1 = _handshakeClient(ctx, p1, 1)
 	})
 	gox(wg, func(ctx context.Context) {
-		err2 = handshake(ctx, p2, 2)
+		err2 = _handshakeServer(ctx, p2, 2)
 	})
 	xwait(wg)
 	xclose(p1)
 	xclose(p2)
 
-	err1Want := "pipe - pipe: handshake: protocol version mismatch: peer = 00000002  ; our side = 00000001"
-	err2Want := "pipe - pipe: handshake: protocol version mismatch: peer = 00000001  ; our side = 00000002"
+	err1Want := "pipe - pipe: handshake (client): protocol version mismatch: peer = 00000002  ; our side = 00000001"
+	err2Want := "pipe - pipe: handshake (server): protocol version mismatch: peer = 00000001  ; our side = 00000002"
 
 	if !(err1 != nil && err1.Error() == err1Want) {
 		t.Errorf("handshake ver mismatch: p1: unexpected error:\nhave: %v\nwant: %v", err1, err1Want)
@@ -73,12 +81,12 @@ func TestHandshake(t *testing.T) {
 		t.Errorf("handshake ver mismatch: p2: unexpected error:\nhave: %v\nwant: %v", err2, err2Want)
 	}
 
-	// tx & rx problem
+	// tx & rx problem (client)
 	p1, p2 = net.Pipe()
-	err1, err2 = nil, nil
+	var err error
 	wg = xsync.NewWorkGroup(bg)
 	gox(wg, func(ctx context.Context) {
-		err1 = handshake(ctx, p1, 1)
+		err = _handshakeClient(ctx, p1, 1)
 	})
 	gox(wg, func(_ context.Context) {
 		xclose(p2)
@@ -86,18 +94,34 @@ func TestHandshake(t *testing.T) {
 	xwait(wg)
 	xclose(p1)
 
-	err11, ok := err1.(*_HandshakeError)
+	err_, ok := err.(*_HandshakeError)
+	if !ok || !(errors.Is(err_.Err, io.ErrClosedPipe /* on Write */) || errors.Is(err_.Err, io.ErrUnexpectedEOF /* on Read */)) {
+		t.Errorf("handshake peer close: unexpected error: %#v", err)
+	}
+
+	// tx & rx problem (server)
+	p1, p2 = net.Pipe()
+	wg = xsync.NewWorkGroup(bg)
+	gox(wg, func(_ context.Context) {
+		xclose(p1)
+	})
+	gox(wg, func(ctx context.Context) {
+		err = _handshakeServer(ctx, p2, 1)
+	})
+	xwait(wg)
+	xclose(p2)
 
-	if !ok || !(err11.Err == io.ErrClosedPipe /* on Write */ || err11.Err == io.ErrUnexpectedEOF /* on Read */) {
-		t.Errorf("handshake peer close: unexpected error: %#v", err1)
+	err_, ok = err.(*_HandshakeError)
+	if !ok || !(errors.Is(err_.Err, io.ErrClosedPipe /* on Write */) || errors.Is(err_.Err, io.ErrUnexpectedEOF /* on Read */)) {
+		t.Errorf("handshake peer close: unexpected error: %#v", err)
 	}
 
-	// ctx cancel
+	// ctx cancel (client)
 	p1, p2 = net.Pipe()
 	ctx, cancel := context.WithCancel(bg)
 	wg = xsync.NewWorkGroup(ctx)
 	gox(wg, func(ctx context.Context) {
-		err1 = handshake(ctx, p1, 1)
+		err = _handshakeClient(ctx, p1, 1)
 	})
 	tdelay()
 	cancel()
@@ -105,10 +129,26 @@ func TestHandshake(t *testing.T) {
 	xclose(p1)
 	xclose(p2)
 
-	err11, ok = err1.(*_HandshakeError)
-
-	if !ok || !(err11.Err == context.Canceled) {
-		t.Errorf("handshake cancel: unexpected error: %#v", err1)
+	err_, ok = err.(*_HandshakeError)
+	if !ok || !(err_.Err == context.Canceled) {
+		t.Errorf("handshake (client): cancel: unexpected error: %#v", err)
 	}
 
+	// ctx cancel (server)
+	p1, p2 = net.Pipe()
+	ctx, cancel = context.WithCancel(bg)
+	wg = xsync.NewWorkGroup(ctx)
+	gox(wg, func(ctx context.Context) {
+		err = _handshakeServer(ctx, p2, 1)
+	})
+	tdelay()
+	cancel()
+	xwait(wg)
+	xclose(p1)
+	xclose(p2)
+
+	err_, ok = err.(*_HandshakeError)
+	if !ok || !(err_.Err == context.Canceled) {
+		t.Errorf("handshake (server): cancel: unexpected error: %#v", err)
+	}
 }