Repository: hyperledger/firefly
Pull request #191 (2021-09-13 14:09:44)

Gracefully shutdown http server when resetting config
Hopefully this should fix https://github.com/hyperledger-labs/firefly-cli/issues/80

The behavior in that issue has been very difficult to reproduce, but this code should help. Even if it doesn't fix that issue, it will not hurt anything else, and it helps prevent other http requests from possibly getting interrupted.


Patch:
diff --git a/Makefile b/Makefile
index e834a85b0..fa02ae29c 100644
--- a/Makefile
+++ b/Makefile
@@ -59,6 +59,7 @@ $(eval $(call makemock, internal/assets,           Manager,        assetmocks))
 $(eval $(call makemock, internal/wsclient,         WSClient,       wsmocks))
 $(eval $(call makemock, internal/orchestrator,     Orchestrator,   orchestratormocks))
 $(eval $(call makemock, internal/apiserver,        Server,         apiservermocks))
+$(eval $(call makemock, internal/apiserver,        IServer,        apiservermocks))
 $(eval $(call makemock, internal/txcommon,         Helper,         txcommonmocks))
 
 firefly-nocgo: ${GOFILES}		
diff --git a/internal/apiserver/http_server.go b/internal/apiserver/http_server.go
index 313a8e09d..461c19165 100644
--- a/internal/apiserver/http_server.go
+++ b/internal/apiserver/http_server.go
@@ -55,9 +55,16 @@ const (
 	HTTPConfTLSKeyFile = "tls.keyFile"
 )
 
+type IServer interface {
+	Close() error
+	Serve(l net.Listener) error
+	ServeTLS(l net.Listener, certFile, keyFile string) error
+	Shutdown(ctx context.Context) error
+}
+
 type httpServer struct {
 	name        string
-	s           *http.Server
+	s           IServer
 	l           net.Listener
 	conf        config.Prefix
 	onClose     chan error
@@ -105,7 +112,7 @@ func (hs *httpServer) createListener(ctx context.Context) (net.Listener, error)
 	return listener, err
 }
 
-func (hs *httpServer) createServer(ctx context.Context, r *mux.Router) (srv *http.Server, err error) {
+func (hs *httpServer) createServer(ctx context.Context, r *mux.Router) (srv IServer, err error) {
 
 	// Support client auth
 	clientAuth := tls.NoClientCert
@@ -165,7 +172,12 @@ func (hs *httpServer) serveHTTP(ctx context.Context) {
 		select {
 		case <-ctx.Done():
 			log.L(ctx).Infof("API server context cancelled - shutting down")
-			hs.s.Close()
+			shutdownContext, cancel := context.WithTimeout(context.Background(), config.GetDuration(config.APIShutdownTimeout))
+			defer cancel()
+			if err := hs.s.Shutdown(shutdownContext); err != nil {
+				hs.onClose <- err
+				return
+			}
 		case <-serverEnded:
 			return
 		}
diff --git a/internal/config/config.go b/internal/config/config.go
index f8a7dd208..cd10fc2fc 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -51,6 +51,8 @@ var (
 	APIRequestTimeout = rootKey("api.requestTimeout")
 	// APIRequestMaxTimeout is the maximum timeout an application can set using a Request-Timeout header
 	APIRequestMaxTimeout = rootKey("api.requestMaxTimeout")
+	// APIShutdownTimeout is the amount of time to wait for any in-flight requests to finish before killing the HTTP server
+	APIShutdownTimeout = rootKey("api.shutdownTimeout")
 	// BatchManagerReadPageSize is the size of each page of messages read from the database into memory when assembling batches
 	BatchManagerReadPageSize = rootKey("batch.manager.readPageSize")
 	// BatchManagerReadPollTimeout is how long without any notifications of new messages to wait, before doing a page query
@@ -269,6 +271,7 @@ func Reset() {
 	viper.SetDefault(string(APIMaxFilterLimit), 250)
 	viper.SetDefault(string(APIMaxFilterSkip), 1000) // protects database (skip+limit pagination is not for bulk operations)
 	viper.SetDefault(string(APIRequestTimeout), "120s")
+	viper.SetDefault(string(APIShutdownTimeout), "10s")
 	viper.SetDefault(string(BatchManagerReadPageSize), 100)
 	viper.SetDefault(string(BatchManagerReadPollTimeout), "30s")
 	viper.SetDefault(string(BatchRetryFactor), 2.0)
diff --git a/internal/database/difactory/plugins_cgo.go b/internal/database/difactory/plugins_cgo.go
index ea82a6123..047114ca4 100644
--- a/internal/database/difactory/plugins_cgo.go
+++ b/internal/database/difactory/plugins_cgo.go
@@ -14,6 +14,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+//go:build cgo
 // +build cgo
 
 package difactory
diff --git a/internal/database/sqlite3/config.go b/internal/database/sqlite3/config.go
index 49762745f..ba9883a36 100644
--- a/internal/database/sqlite3/config.go
+++ b/internal/database/sqlite3/config.go
@@ -14,6 +14,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+//go:build cgo
 // +build cgo
 
 package sqlite3
diff --git a/internal/database/sqlite3/sqlite3.go b/internal/database/sqlite3/sqlite3.go
index 6613fd4d8..dd2196213 100644
--- a/internal/database/sqlite3/sqlite3.go
+++ b/internal/database/sqlite3/sqlite3.go
@@ -14,6 +14,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+//go:build cgo
 // +build cgo
 
 package sqlite3
diff --git a/mocks/apiservermocks/i_server.go b/mocks/apiservermocks/i_server.go
new file mode 100644
index 000000000..c1c48e8ca
--- /dev/null
+++ b/mocks/apiservermocks/i_server.go
@@ -0,0 +1,71 @@
+// Code generated by mockery v1.0.0. DO NOT EDIT.
+
+package apiservermocks
+
+import (
+	context "context"
+	net "net"
+
+	mock "github.com/stretchr/testify/mock"
+)
+
+// IServer is an autogenerated mock type for the IServer type
+type IServer struct {
+	mock.Mock
+}
+
+// Close provides a mock function with given fields:
+func (_m *IServer) Close() error {
+	ret := _m.Called()
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func() error); ok {
+		r0 = rf()
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}
+
+// Serve provides a mock function with given fields: l
+func (_m *IServer) Serve(l net.Listener) error {
+	ret := _m.Called(l)
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func(net.Listener) error); ok {
+		r0 = rf(l)
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}
+
+// ServeTLS provides a mock function with given fields: l, certFile, keyFile
+func (_m *IServer) ServeTLS(l net.Listener, certFile string, keyFile string) error {
+	ret := _m.Called(l, certFile, keyFile)
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func(net.Listener, string, string) error); ok {
+		r0 = rf(l, certFile, keyFile)
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}
+
+// Shutdown provides a mock function with given fields: ctx
+func (_m *IServer) Shutdown(ctx context.Context) error {
+	ret := _m.Called(ctx)
+
+	var r0 error
+	if rf, ok := ret.Get(0).(func(context.Context) error); ok {
+		r0 = rf(ctx)
+	} else {
+		r0 = ret.Error(0)
+	}
+
+	return r0
+}


Test patch:
diff --git a/internal/apiserver/http_server_test.go b/internal/apiserver/http_server_test.go
index 4d2195490..8db6d2ed2 100644
--- a/internal/apiserver/http_server_test.go
+++ b/internal/apiserver/http_server_test.go
@@ -25,6 +25,7 @@ import (
 	"crypto/x509/pkix"
 	"encoding/json"
 	"encoding/pem"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"math/big"
@@ -36,7 +37,9 @@ import (
 
 	"github.com/gorilla/mux"
 	"github.com/hyperledger/firefly/internal/config"
+	"github.com/hyperledger/firefly/mocks/apiservermocks"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
 )
 
 func TestInvalidListener(t *testing.T) {
@@ -60,6 +63,43 @@ func TestServeFail(t *testing.T) {
 	assert.Error(t, err)
 }
 
+func TestShutdownOk(t *testing.T) {
+	config.Reset()
+	cp := config.NewPluginConfig("ut")
+	initHTTPConfPrefx(cp, 0)
+	errChan := make(chan error)
+	ctx, cancel := context.WithCancel(context.Background())
+	_, err := newHTTPServer(ctx, "ut", mux.NewRouter(), errChan, cp)
+	assert.NoError(t, err)
+	cancel()
+}
+
+func TestShutdownError(t *testing.T) {
+	testDone := make(chan struct{})
+	config.Reset()
+	cp := config.NewPluginConfig("ut")
+	config.Set(config.APIShutdownTimeout, "1ms")
+	initHTTPConfPrefx(cp, 0)
+	errChan := make(chan error)
+	ctx, cancel := context.WithCancel(context.Background())
+	s, err := newHTTPServer(ctx, "ut", mux.NewRouter(), errChan, cp)
+	assert.NoError(t, err)
+	m := &apiservermocks.IServer{}
+	m.On("Shutdown", mock.Anything).Return(errors.New("forced error"))
+	m.On("Serve", mock.Anything).Return(nil).Run(func(args mock.Arguments) {
+		<-testDone
+		return
+	})
+	s.s = m
+	go func() {
+		s.serveHTTP(ctx)
+	}()
+	cancel()
+	err = <-errChan
+	assert.Error(t, err)
+	testDone <- struct{}{}
+}
+
 func TestMissingCAFile(t *testing.T) {
 	cp := config.NewPluginConfig("ut")
 	initHTTPConfPrefx(cp, 0)