fix: efficient error handling in manager-api including graceful shutd…

…own, self contained methods. (#1814)

api/cmd/managerapi.go

@@ -101,7 +101,7 @@ func manageAPI() error {
 
 	if err := utils.WritePID(conf.PIDPath); err != nil {
 		log.Errorf("failed to write pid: %s", err)
-		panic(err)
+		return err
 	}
 	utils.AppendToClosers(func() error {
 		if err := os.Remove(conf.PIDPath); err != nil {
@@ -111,6 +111,15 @@ func manageAPI() error {
 		return nil
 	})
 
+	// Signal received to the process externally.
+	quit := make(chan os.Signal, 1)
+	signal.Notify(quit, syscall.SIGINT, syscall.SIGTERM)
+
+	defer func() {
+		utils.CloseAll()
+		signal.Stop(quit)
+	}()
+
 	droplet.Option.Orchestrator = func(mws []droplet.Middleware) []droplet.Middleware {
 		var newMws []droplet.Middleware
 		// default middleware order: resp_reshape, auto_input, traffic_log
@@ -122,17 +131,21 @@ func manageAPI() error {
 
 	if err := storage.InitETCDClient(conf.ETCDConfig); err != nil {
 		log.Errorf("init etcd client fail: %w", err)
-		panic(err)
+		return err
 	}
 	if err := store.InitStores(); err != nil {
 		log.Errorf("init stores fail: %w", err)
-		panic(err)
+		return err
 	}
 
+	var server, serverSSL *http.Server
+	// For internal error handling across multiple goroutines.
+	errsig := make(chan error, 1)
+
 	// routes
 	r := internal.SetUpRouter()
-	addr := fmt.Sprintf("%s:%d", conf.ServerHost, conf.ServerPort)
-	s := &http.Server{
+	addr := net.JoinHostPort(conf.ServerHost, strconv.Itoa(conf.ServerPort))
+	server = &http.Server{
 		Addr:         addr,
 		Handler:      r,
 		ReadTimeout:  time.Duration(1000) * time.Millisecond,
@@ -141,21 +154,17 @@ func manageAPI() error {
 
 	log.Infof("The Manager API is listening on %s", addr)
 
-	quit := make(chan os.Signal, 1)
-	signal.Notify(quit, syscall.SIGINT, syscall.SIGTERM)
-	defer signal.Stop(quit)
-
 	go func() {
-		if err := s.ListenAndServe(); err != nil && err != http.ErrServerClosed {
-			utils.CloseAll()
-			log.Fatalf("listen and serv fail: %s", err)
+		if err := server.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+			log.Errorf("listen and serv fail: %s", err)
+			errsig <- err
 		}
 	}()
 
 	// HTTPS
 	if conf.SSLCert != "" && conf.SSLKey != "" {
 		addrSSL := net.JoinHostPort(conf.ServerHost, strconv.Itoa(conf.SSLPort))
-		serverSSL := &http.Server{
+		serverSSL = &http.Server{
 			Addr:         addrSSL,
 			Handler:      r,
 			ReadTimeout:  time.Duration(1000) * time.Millisecond,
@@ -169,28 +178,37 @@ func manageAPI() error {
 		go func() {
 			err := serverSSL.ListenAndServeTLS(conf.SSLCert, conf.SSLKey)
 			if err != nil && err != http.ErrServerClosed {
-				utils.CloseAll()
-				log.Fatalf("listen and serve for HTTPS failed: %s", err)
+				log.Errorf("listen and serve for HTTPS failed: %s", err)
+				errsig <- err
 			}
 		}()
 	}
 
 	printInfo()
 
-	sig := <-quit
-	log.Infof("The Manager API server receive %s and start shutting down", sig.String())
+	select {
+	case err := <-errsig:
+		return err
 
-	ctx, cancel := context.WithTimeout(context.TODO(), 5*time.Second)
-	defer cancel()
+	case sig := <-quit:
+		log.Infof("The Manager API server receive %s and start shutting down", sig.String())
 
-	if err := s.Shutdown(ctx); err != nil {
-		log.Errorf("Shutting down server error: %s", err)
+		shutdownServer(server)
+		shutdownServer(serverSSL)
+		log.Infof("The Manager API server exited")
+		return nil
 	}
+}
 
-	log.Infof("The Manager API server exited")
+func shutdownServer(server *http.Server) {
+	if server != nil {
+		ctx, cancel := context.WithTimeout(context.TODO(), 5*time.Second)
+		defer cancel()
 
-	utils.CloseAll()
-	return nil
+		if err := server.Shutdown(ctx); err != nil {
+			log.Errorf("Shutting down server error: %s", err)
+		}
+	}
 }
 
 func newStartCommand() *cobra.Command {

api/internal/core/store/store.go

@@ -21,6 +21,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"os"
 	"reflect"
 	"sort"
 	"sync"
@@ -105,6 +106,7 @@ func (s *GenericStore) Init() error {
 		key := ret[i].Key[len(s.opt.BasePath)+1:]
 		objPtr, err := s.StringToObjPtr(ret[i].Value, key)
 		if err != nil {
+			fmt.Fprintln(os.Stderr, "Error occurred while initializing logical store: ", s.opt.BasePath)
 			return err
 		}
 
@@ -353,8 +355,8 @@ func (s *GenericStore) StringToObjPtr(str, key string) (interface{}, error) {
 	ret := objPtr.Interface()
 	err := json.Unmarshal([]byte(str), ret)
 	if err != nil {
-		log.Errorf("json marshal failed: %s", err)
-		return nil, fmt.Errorf("json unmarshal failed: %s", err)
+		log.Errorf("json unmarshal failed: %s", err)
+		return nil, fmt.Errorf("json unmarshal failed\n\tRelated Key:\t\t%s\n\tError Description:\t%s", key, err)
 	}
 
 	if setter, ok := ret.(entity.BaseInfoSetter); ok {

api/internal/core/store/store_test.go

@@ -205,7 +205,8 @@ func TestGenericStore_Init(t *testing.T) {
 					Value: `{"Field1":"demo2-f1", "Field2":"demo2-f2"}`,
 				},
 			},
-			wantErr:        fmt.Errorf("json unmarshal failed: invalid character ',' after object key"),
+			wantErr: fmt.Errorf("json unmarshal failed\n\tRelated Key:\t\tdemo1-f1\n\tError Description:\t" +
+				"invalid character ',' after object key"),
 			wantListCalled: true,
 		},
 	}

api/internal/utils/pid.go

@@ -26,6 +26,9 @@ import (
 
 // WritePID write pid to the given file path.
 func WritePID(filepath string) error {
+	if _, err := os.Stat(filepath); err == nil {
+		return fmt.Errorf("instance of Manager API already running: a pid file exists in %s", filepath)
+	}
 	pid := os.Getpid()
 	f, err := os.OpenFile(filepath, os.O_WRONLY|os.O_CREATE|os.O_TRUNC|os.O_CREATE, 0600)
 	if err != nil {

api/test/shell/cli_test.sh

@@ -487,6 +487,59 @@ fi
 sleep 6
 clean_up
 
+# run manager api as os service
+# 2 times OK for installing and starting
+if [[ `echo $(sudo ./manager-api start) | grep -o "OK" | wc -l` -ne "2" ]]; then
+  echo "error while initializing the service"
+  exit 1
+fi
+# check running status
+if [[ `echo $(sudo ./manager-api status) | grep -c "running..."` -ne "1" ]]; then
+  echo "error while starting the service"
+  exit 1
+fi
+# stop the service
+sudo ./manager-api stop
+sleep 2
+# recheck running status
+if [[ `echo $(sudo ./manager-api status) | grep -c "Service is stopped"` -ne "1" ]]; then
+  echo "error while stopping the service"
+  exit 1
+fi
+# restart the service
+# 1 time OK for just for starting
+if [[ `echo $(sudo ./manager-api start) | grep -c "OK"` -ne "1" ]]; then
+  echo "error while restarting the service"
+  exit 1
+fi
+# stop the service
+sudo ./manager-api stop
+sleep 2
+# remove the service
+if [[ `echo $(sudo ./manager-api remove) | grep -c "OK"` -ne "1" ]]; then
+  echo "error while removing the service"
+  exit 1
+fi
+# test manager-api output for bad data on etcd
+# make a dummy entry
+./etcd-v3.4.14-linux-amd64/etcdctl put /apisix/routes/unique1 "{\"id\":}"
+sleep 2
+
+./manager-api 2>man-api.err &
+sleep 4
+
+if [[ `cat man-api.err | grep -c "Error occurred while initializing logical store:  /apisix/routes"` -ne '1' ||
+`cat man-api.err | grep -c "Error: json unmarshal failed"` -ne '1' ]];then
+  echo "manager api failed to stream error on stderr for bad data"
+  exit 1
+fi
+# delete dummy entry
+./etcd-v3.4.14-linux-amd64/etcdctl del /apisix/routes/unique1
+# just to make sure
+./manager-api stop
+sleep 6
+clean_up
+
 # run manager api as os service
 # 2 times OK for installing and starting
 if [[ `echo $(sudo ./manager-api start) | grep -o "OK" | wc -l` -ne "2" ]]; then