Merge pull request #97 from telekom-mms/feature/avoid-socket-api-erro…

…rs-during-shutdown

Avoid Socket API errors in vpncscript during Daemon shutdown

internal/api/server.go

@@ -12,11 +12,18 @@ import (
 	log "github.com/sirupsen/logrus"
 )
 
+const (
+	// ServerShuttingDown is the error reply to client requests when the
+	// API server is shutting down.
+	ServerShuttingDown = "server is shutting down"
+)
+
 // Server is a Daemon API server.
 type Server struct {
 	config   *Config
 	listen   net.Listener
 	requests chan *Request
+	shutdown chan struct{}
 	done     chan struct{}
 	closed   chan struct{}
 }
@@ -31,6 +38,17 @@ func (s *Server) isStopping() bool {
 	}
 }
 
+// sendShuttingDownError sends the "shutting down" error to the client and
+// closes the connection.
+func (s *Server) sendShuttingDownError(conn net.Conn) {
+	// tell client server is not accepting connections anymore
+	e := NewError([]byte(ServerShuttingDown))
+	if err := WriteMessage(conn, e); err != nil {
+		log.WithError(err).Error("Daemon message send error")
+	}
+	_ = conn.Close()
+}
+
 // handleRequest handles a request from the client.
 func (s *Server) handleRequest(conn net.Conn) {
 	// set timeout for entire request/response exchange
@@ -68,7 +86,10 @@ func (s *Server) handleRequest(conn net.Conn) {
 		msg:  msg,
 		conn: conn,
 	}:
+	case <-s.shutdown:
+		s.sendShuttingDownError(conn)
 	case <-s.done:
+		s.sendShuttingDownError(conn)
 	}
 }
 
@@ -205,6 +226,13 @@ func (s *Server) Stop() {
 	<-s.closed
 }
 
+// Shutdown stops accepting client requests in the API server. This is called
+// before Stop to tell clients the server is shutting down. Client requests
+// should not be read from the requests channel after calling this.
+func (s *Server) Shutdown() {
+	close(s.shutdown)
+}
+
 // Requests returns the clients channel.
 func (s *Server) Requests() chan *Request {
 	return s.requests
@@ -215,6 +243,7 @@ func NewServer(config *Config) *Server {
 	return &Server{
 		config:   config,
 		requests: make(chan *Request),
+		shutdown: make(chan struct{}),
 		done:     make(chan struct{}),
 		closed:   make(chan struct{}),
 	}

internal/api/server_test.go

@@ -54,6 +54,25 @@ func TestServerHandleRequest(t *testing.T) {
 	if req.conn != c1 {
 		t.Errorf("got %p, want %p", req.conn, c1)
 	}
+
+	// valid request during shutdown
+	server.Shutdown()
+	c1, c2 = net.Pipe()
+	go server.handleRequest(c1)
+
+	msg = &Message{Header: Header{Type: TypeVPNConfigUpdate}}
+	if err := WriteMessage(c2, msg); err != nil {
+		t.Fatal(err)
+	}
+
+	msg, err := ReadMessage(c2)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if msg.Header.Type != TypeError || string(msg.Value) != ServerShuttingDown {
+		t.Error("unexpected reply")
+	}
 }
 
 // TestServerSetSocketOwner tests setSocketOwner of Server.
@@ -118,6 +137,7 @@ func TestServerStartStop(t *testing.T) {
 	if err := server.Start(); err != nil {
 		t.Error(err)
 	}
+	server.Shutdown()
 	server.Stop()
 }
 
@@ -138,6 +158,7 @@ func TestNewServer(t *testing.T) {
 
 	if server == nil ||
 		server.requests == nil ||
+		server.shutdown == nil ||
 		server.done == nil ||
 		server.closed == nil {
 		t.Errorf("got nil, want != nil")

internal/daemon/daemon.go

@@ -671,11 +671,12 @@ func (d *Daemon) start() {
 	defer d.stopTrafPol()
 	defer d.stopTND()
 	defer d.vpnsetup.Stop()
+	defer d.server.Stop()
 	defer d.handleRunnerDisconnect() // clean up vpn config
 	defer d.runner.Stop()
-	defer d.server.Stop()
 	defer d.dbus.Stop()
 	defer d.profmon.Stop()
+	defer d.server.Shutdown()
 
 	// run main loop
 	for {

internal/vpncscript/client.go

@@ -44,8 +44,12 @@ func runClient(socketFile string, configUpdate *daemon.VPNConfigUpdate) error {
 		log.WithField("reply", reply.Value).
 			Debug("VPNCScript received OK reply from Daemon")
 	case api.TypeError:
-		log.WithField("error", string(reply.Value)).
-			Error("VPNCScript received error reply from Daemon")
+		e := string(reply.Value)
+		if e == api.ServerShuttingDown {
+			// skip logging error when server is shutting down
+			return nil
+		}
+		log.WithField("error", e).Error("VPNCScript received error reply from Daemon")
 	}
 	return nil
 }

internal/vpncscript/client_test.go

@@ -29,6 +29,7 @@ func TestRunClient(t *testing.T) {
 	if err := runClient(sockfile, &daemon.VPNConfigUpdate{}); err != nil {
 		t.Fatal(err)
 	}
+	server.Shutdown()
 	server.Stop()
 
 	// with error reply
@@ -45,6 +46,18 @@ func TestRunClient(t *testing.T) {
 	if err := runClient(sockfile, &daemon.VPNConfigUpdate{}); err != nil {
 		t.Fatal(err)
 	}
+	server.Shutdown()
+	server.Stop()
+
+	// with "shutting down" error reply
+	server = api.NewServer(config)
+	if err := server.Start(); err != nil {
+		t.Fatal(err)
+	}
+	server.Shutdown()
+	if err := runClient(sockfile, &daemon.VPNConfigUpdate{}); err != nil {
+		t.Fatal(err)
+	}
 	server.Stop()
 
 	// helper for config update creation
@@ -86,5 +99,6 @@ func TestRunClient(t *testing.T) {
 			t.Errorf("length %d returned error: %v", length, err)
 		}
 	}
+	server.Shutdown()
 	server.Stop()
 }