Improve server start and stop logic

internal/servercore/api.go

@@ -23,9 +23,10 @@ import (
 )
 
 type Server struct {
-	conf      *server.Configuration
-	sessions  sessionStore
-	scheduler *gocron.Scheduler
+	conf          *server.Configuration
+	sessions      sessionStore
+	scheduler     *gocron.Scheduler
+	stopScheduler chan bool
 }
 
 func New(conf *server.Configuration) (*Server, error) {
@@ -41,11 +42,16 @@ func New(conf *server.Configuration) (*Server, error) {
 	s.scheduler.Every(10).Seconds().Do(func() {
 		s.sessions.deleteExpired()
 	})
-	s.scheduler.Start()
+	s.stopScheduler = s.scheduler.Start()
 
 	return s, s.verifyConfiguration(s.conf)
 }
 
+func (s *Server) Stop() {
+	s.stopScheduler <- true
+	s.sessions.stop()
+}
+
 func (s *Server) verifyConfiguration(configuration *server.Configuration) error {
 	if s.conf.Logger == nil {
 		s.conf.Logger = logrus.New()

internal/servercore/sessions.go

@@ -42,6 +42,7 @@ type sessionStore interface {
 	add(session *session)
 	update(session *session)
 	deleteExpired()
+	stop()
 }
 
 type memorySessionStore struct {
@@ -89,6 +90,16 @@ func (s *memorySessionStore) update(session *session) {
 	session.onUpdate()
 }
 
+func (s *memorySessionStore) stop() {
+	s.Lock()
+	defer s.Unlock()
+	for _, session := range s.requestor {
+		if session.evtSource != nil {
+			session.evtSource.Close()
+		}
+	}
+}
+
 func (s *memorySessionStore) deleteExpired() {
 	// First check which sessions have expired
 	// We don't need a write lock for this yet, so postpone that for actual deleting

internal/sessiontest/server_test.go

@@ -42,7 +42,7 @@ func StartRequestorServer(configuration *requestorserver.Configuration) {
 }
 
 func StopRequestorServer() {
-	_ = requestorServer.Stop()
+	requestorServer.Stop()
 }
 
 func StartIrmaServer(t *testing.T) {

server/irmaserver/main.go

@@ -45,6 +45,14 @@ func New(conf *server.Configuration) (*Server, error) {
 	}, nil
 }
 
+// Stop the server.
+func Stop() {
+	s.Stop()
+}
+func (s *Server) Stop() {
+	s.Server.Stop()
+}
+
 // StartSession starts an IRMA session, running the handler on completion, if specified.
 // The session token (the second return parameter) can be used in GetSessionResult()
 // and CancelSession().

server/requestorserver/conf.go

@@ -202,7 +202,7 @@ func (conf *Configuration) initialize() error {
 		return errors.Errorf("client_port must be between 0 and 65535 (was %d)", conf.ClientPort)
 	}
 	if conf.ClientListenAddress != "" && conf.ClientPort == 0 {
-		return errors.New("client_listen_addr must be combined with a nonzero clientport")
+		return errors.New("client_listen_addr must be combined with a nonzero client_port")
 	}
 
 	tlsConf, err := conf.tlsConfig()

server/requestorserver/server.go

@@ -5,6 +5,7 @@
 package requestorserver
 
 import (
+	"context"
 	"crypto/tls"
 	"crypto/x509"
 	"encoding/json"
@@ -28,9 +29,9 @@ import (
 
 // Server is a requestor server instance.
 type Server struct {
-	serv, clientserv *http.Server
-	conf             *Configuration
-	irmaserv         *irmaserver.Server
+	conf     *Configuration
+	irmaserv *irmaserver.Server
+	stop     chan struct{}
 }
 
 // Start the server. If successful then it will not return until Stop() is called.
@@ -42,64 +43,92 @@ func (s *Server) Start(config *Configuration) error {
 		s.conf.Logger.Debug("Configuration: ", string(bts), "\n")
 	}
 
-	// Start server(s)
+	// We start either one or two servers, depending on whether a separate client server is enabled, such that:
+	// - if any of them returns, the other is also stopped (neither of them is of use without the other)
+	// - if any of them returns an unexpected error (ie. other than http.ErrServerClosed), the error is logged and returned
+	// - we have a way of stopping all servers from outside (with Stop())
+	// - the function returns only after all servers have been stopped
+	// - any unexpected error is dealt with here instead of when stopping using Stop().
+	// Inspired by https://dave.cheney.net/practical-go/presentations/qcon-china.html#_never_start_a_goroutine_without_when_it_will_stop
+
+	count := 1
 	if s.conf.separateClientServer() {
-		go s.startClientServer()
+		count = 2
+	}
+	done := make(chan error, count)
+	s.stop = make(chan struct{})
+
+	if s.conf.separateClientServer() {
+		go func() {
+			done <- s.startClientServer()
+		}()
+	}
+	go func() {
+		done <- s.startRequestorServer()
+	}()
+
+	var stopped bool
+	var err error
+	for i := 0; i < cap(done); i++ {
+		if err = <-done; err != nil {
+			_ = server.LogError(err)
+		}
+		if !stopped {
+			stopped = true
+			close(s.stop)
+		}
 	}
-	s.startRequestorServer()
 
-	return nil
+	return err
 }
 
-func (s *Server) startRequestorServer() {
-	s.serv = &http.Server{}
+func (s *Server) startRequestorServer() error {
 	tlsConf, _ := s.conf.tlsConfig()
-	s.startServer(s.serv, s.Handler(), "Server", s.conf.ListenAddress, s.conf.Port, tlsConf)
+	return s.startServer(s.Handler(), "Server", s.conf.ListenAddress, s.conf.Port, tlsConf)
 }
 
-func (s *Server) startClientServer() {
-	s.clientserv = &http.Server{}
+func (s *Server) startClientServer() error {
 	tlsConf, _ := s.conf.clientTlsConfig()
-	s.startServer(s.clientserv, s.ClientHandler(), "Client server", s.conf.ClientListenAddress, s.conf.ClientPort, tlsConf)
+	return s.startServer(s.ClientHandler(), "Client server", s.conf.ClientListenAddress, s.conf.ClientPort, tlsConf)
 }
 
-func (s *Server) startServer(serv *http.Server, handler http.Handler, name, addr string, port int, tlsConf *tls.Config) {
+func (s *Server) startServer(handler http.Handler, name, addr string, port int, tlsConf *tls.Config) error {
 	fulladdr := fmt.Sprintf("%s:%d", addr, port)
 	s.conf.Logger.Info(name, " listening at ", fulladdr)
-	serv.Addr = fulladdr
-	serv.Handler = handler
-	var err error
+
+	serv := &http.Server{
+		Addr:      fulladdr,
+		Handler:   handler,
+		TLSConfig: tlsConf,
+	}
+
+	go func() {
+		<-s.stop
+		if err := serv.Shutdown(context.Background()); err != nil {
+			_ = server.LogError(err)
+		}
+	}()
+
 	if tlsConf != nil {
-		serv.TLSConfig = tlsConf
 		// Disable HTTP/2 (see package documentation of http): it breaks server side events :(
 		serv.TLSNextProto = make(map[string]func(*http.Server, *tls.Conn, http.Handler))
 		s.conf.Logger.Info(name, " TLS enabled")
-		err = serv.ListenAndServeTLS("", "")
+		return filterStopError(serv.ListenAndServeTLS("", ""))
 	} else {
-		err = serv.ListenAndServe()
-	}
-	if err != http.ErrServerClosed {
-		_ = server.LogFatal(err)
+		return filterStopError(serv.ListenAndServe())
 	}
 }
 
-func (s *Server) Stop() error {
-	var err1, err2 error
-
-	// Even if closing serv fails, we want to try closing clientserv
-	err1 = s.serv.Close()
-	if s.clientserv != nil {
-		err2 = s.clientserv.Close()
+func filterStopError(err error) error {
+	if err == http.ErrServerClosed {
+		return nil
 	}
+	return err
+}
 
-	// Now check errors
-	if err1 != nil && err1 != http.ErrServerClosed {
-		return err1
-	}
-	if err2 != nil && err2 != http.ErrServerClosed {
-		return err2
-	}
-	return nil
+func (s *Server) Stop() {
+	s.irmaserv.Stop()
+	s.stop <- struct{}{}
 }
 
 func New(config *Configuration) (*Server, error) {