Repository: beatlabs/patron
Pull request #247 (2020-08-06 13:48:10)

Fix the bug with SIGTERM non-graceful handling
Signed-off-by: Alex Demin <a.demin@thebeat.co>

<!--
Thanks for taking precious time for making a PR.

Before creating a pull request, please make sure:
- Your PR solves one problem for which a issue exist and a solution has been discussed
- You have read the guide for contributing
  - See https://github.com/beatlabs/patron/blob/master/CONTRIBUTING.md
- You signed all your commits (otherwise we won't be able to merge the PR)
  - See https://github.com/beatlabs/patron/blob/master/CONTRIBUTING.md#sign-your-work
- You added unit tests for the new functionality
- You mention in the PR description which issue it is addressing, e.g. "Resolves #123"
-->

## Which problem is this PR solving?

This PR closes https://github.com/beatlabs/patron/issues/246

## Short description of the changes

- Replaces cancelled context in http server `Shutdown` with configurable timed context (default shutdown grace period is 5 seconds)
- Removes global `BaseContext` from http service.

codecov[bot]: # [Codecov](https://codecov.io/gh/beatlabs/patron/pull/247?src=pr&el=h1) Report
> Merging [#247](https://codecov.io/gh/beatlabs/patron/pull/247?src=pr&el=desc) into [master](https://codecov.io/gh/beatlabs/patron/commit/b65270a3747532620b7d13ffcc011f9ad3cc4938&el=desc) will **increase** coverage by `0.11%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/beatlabs/patron/pull/247/graphs/tree.svg?width=650&height=150&src=pr&token=sxY15rXW1X)](https://codecov.io/gh/beatlabs/patron/pull/247?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #247      +/-   ##
==========================================
+ Coverage   73.16%   73.27%   +0.11%     
==========================================
  Files          61       61              
  Lines        3667     3675       +8     
==========================================
+ Hits         2683     2693      +10     
+ Misses        907      905       -2     
  Partials       77       77              
```


| [Impacted Files](https://codecov.io/gh/beatlabs/patron/pull/247?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [component/http/component.go](https://codecov.io/gh/beatlabs/patron/pull/247/diff?src=pr&el=tree#diff-Y29tcG9uZW50L2h0dHAvY29tcG9uZW50Lmdv) | `97.81% <100.00%> (+0.13%)` | :arrow_up: |
| [component/async/kafka/simple/duration\_client.go](https://codecov.io/gh/beatlabs/patron/pull/247/diff?src=pr&el=tree#diff-Y29tcG9uZW50L2FzeW5jL2thZmthL3NpbXBsZS9kdXJhdGlvbl9jbGllbnQuZ28=) | `96.87% <0.00%> (+3.12%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/beatlabs/patron/pull/247?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/beatlabs/patron/pull/247?src=pr&el=footer). Last update [b65270a...7b941cc](https://codecov.io/gh/beatlabs/patron/pull/247?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/component/http/component.go b/component/http/component.go
index 5455c63c0..7c5d54cc2 100644
--- a/component/http/component.go
+++ b/component/http/component.go
@@ -4,7 +4,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"net"
 	"net/http"
 	"sync"
 	"time"
@@ -15,10 +14,11 @@ import (
 )
 
 const (
-	httpPort         = 50000
-	httpReadTimeout  = 5 * time.Second
-	httpWriteTimeout = 10 * time.Second
-	httpIdleTimeout  = 120 * time.Second
+	httpPort            = 50000
+	httpReadTimeout     = 5 * time.Second
+	httpWriteTimeout    = 10 * time.Second
+	httpIdleTimeout     = 120 * time.Second
+	shutdownGracePeriod = 5 * time.Second
 )
 
 var (
@@ -30,11 +30,12 @@ var (
 
 // Component implementation of HTTP.
 type Component struct {
-	ac               AliveCheckFunc
-	rc               ReadyCheckFunc
-	httpPort         int
-	httpReadTimeout  time.Duration
-	httpWriteTimeout time.Duration
+	ac                  AliveCheckFunc
+	rc                  ReadyCheckFunc
+	httpPort            int
+	httpReadTimeout     time.Duration
+	httpWriteTimeout    time.Duration
+	shutdownGracePeriod time.Duration
 	sync.Mutex
 	routes      []Route
 	middlewares []MiddlewareFunc
@@ -47,14 +48,16 @@ func (c *Component) Run(ctx context.Context) error {
 	c.Lock()
 	log.Debug("applying tracing to routes")
 	chFail := make(chan error)
-	srv := c.createHTTPServer(ctx)
+	srv := c.createHTTPServer()
 	go c.listenAndServe(srv, chFail)
 	c.Unlock()
 
 	select {
 	case <-ctx.Done():
 		log.Info("shutting down component")
-		return srv.Shutdown(ctx)
+		tctx, cancel := context.WithTimeout(context.Background(), c.shutdownGracePeriod)
+		defer cancel()
+		return srv.Shutdown(tctx)
 	case err := <-chFail:
 		return err
 	}
@@ -70,7 +73,7 @@ func (c *Component) listenAndServe(srv *http.Server, ch chan<- error) {
 	ch <- srv.ListenAndServe()
 }
 
-func (c *Component) createHTTPServer(ctx context.Context) *http.Server {
+func (c *Component) createHTTPServer() *http.Server {
 	log.Debugf("adding %d routes", len(c.routes))
 	router := httprouter.New()
 	for _, route := range c.routes {
@@ -93,25 +96,23 @@ func (c *Component) createHTTPServer(ctx context.Context) *http.Server {
 		WriteTimeout: c.httpWriteTimeout,
 		IdleTimeout:  httpIdleTimeout,
 		Handler:      routerAfterMiddleware,
-		BaseContext: func(net.Listener) context.Context {
-			return ctx
-		},
 	}
 }
 
 // Builder gathers all required and optional properties, in order
 // to construct an HTTP component.
 type Builder struct {
-	ac               AliveCheckFunc
-	rc               ReadyCheckFunc
-	httpPort         int
-	httpReadTimeout  time.Duration
-	httpWriteTimeout time.Duration
-	routesBuilder    *RoutesBuilder
-	middlewares      []MiddlewareFunc
-	certFile         string
-	keyFile          string
-	errors           []error
+	ac                  AliveCheckFunc
+	rc                  ReadyCheckFunc
+	httpPort            int
+	httpReadTimeout     time.Duration
+	httpWriteTimeout    time.Duration
+	shutdownGracePeriod time.Duration
+	routesBuilder       *RoutesBuilder
+	middlewares         []MiddlewareFunc
+	certFile            string
+	keyFile             string
+	errors              []error
 }
 
 // NewBuilder initiates the HTTP component builder chain.
@@ -120,13 +121,14 @@ type Builder struct {
 func NewBuilder() *Builder {
 	var errs []error
 	return &Builder{
-		ac:               DefaultAliveCheck,
-		rc:               DefaultReadyCheck,
-		httpPort:         httpPort,
-		httpReadTimeout:  httpReadTimeout,
-		httpWriteTimeout: httpWriteTimeout,
-		routesBuilder:    NewRoutesBuilder(),
-		errors:           errs,
+		ac:                  DefaultAliveCheck,
+		rc:                  DefaultReadyCheck,
+		httpPort:            httpPort,
+		httpReadTimeout:     httpReadTimeout,
+		httpWriteTimeout:    httpWriteTimeout,
+		shutdownGracePeriod: shutdownGracePeriod,
+		routesBuilder:       NewRoutesBuilder(),
+		errors:              errs,
 	}
 }
 
@@ -190,6 +192,18 @@ func (cb *Builder) WithWriteTimeout(wt time.Duration) *Builder {
 	return cb
 }
 
+// WithShutdownGracePeriod sets the Shutdown Grace Period for the HTTP component.
+func (cb *Builder) WithShutdownGracePeriod(gp time.Duration) *Builder {
+	if gp <= 0*time.Second {
+		cb.errors = append(cb.errors, errors.New("negative or zero shutdown grace period provided"))
+	} else {
+		log.Infof("setting shutdown grace period")
+		cb.shutdownGracePeriod = gp
+	}
+
+	return cb
+}
+
 // WithPort sets the port used by the HTTP component.
 func (cb *Builder) WithPort(p int) *Builder {
 	if p <= 0 || p > 65535 {
@@ -243,14 +257,15 @@ func (cb *Builder) Create() (*Component, error) {
 	}
 
 	return &Component{
-		ac:               cb.ac,
-		rc:               cb.rc,
-		httpPort:         cb.httpPort,
-		httpReadTimeout:  cb.httpReadTimeout,
-		httpWriteTimeout: cb.httpWriteTimeout,
-		routes:           routes,
-		middlewares:      cb.middlewares,
-		certFile:         cb.certFile,
-		keyFile:          cb.keyFile,
+		ac:                  cb.ac,
+		rc:                  cb.rc,
+		httpPort:            cb.httpPort,
+		httpReadTimeout:     cb.httpReadTimeout,
+		httpWriteTimeout:    cb.httpWriteTimeout,
+		shutdownGracePeriod: cb.shutdownGracePeriod,
+		routes:              routes,
+		middlewares:         cb.middlewares,
+		certFile:            cb.certFile,
+		keyFile:             cb.keyFile,
 	}, nil
 }


Test patch:
diff --git a/component/http/component_test.go b/component/http/component_test.go
index f52916aa1..19cc8e172 100644
--- a/component/http/component_test.go
+++ b/component/http/component_test.go
@@ -9,6 +9,7 @@ import (
 
 	errs "github.com/beatlabs/patron/errors"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestBuilderWithoutOptions(t *testing.T) {
@@ -63,14 +64,38 @@ func Test_createHTTPServer(t *testing.T) {
 		httpReadTimeout:  5 * time.Second,
 		httpWriteTimeout: 10 * time.Second,
 	}
-	ctx := context.Background()
-	s := cmp.createHTTPServer(ctx)
+	s := cmp.createHTTPServer()
 	assert.NotNil(t, s)
 	assert.Equal(t, ":10000", s.Addr)
 	assert.Equal(t, 5*time.Second, s.ReadTimeout)
 	assert.Equal(t, 10*time.Second, s.WriteTimeout)
 }
 
+func TestBuilder_WithShutdownGracePeriod(t *testing.T) {
+	testCases := map[string]struct {
+		gp     time.Duration
+		expErr string
+	}{
+		"success":     {gp: 10 * time.Second},
+		"wrong value": {gp: -10 * time.Second, expErr: "negative or zero shutdown grace period provided\n"},
+	}
+
+	for name, tt := range testCases {
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()
+			cc, err := NewBuilder().WithShutdownGracePeriod(tt.gp).Create()
+			if tt.expErr != "" {
+				assert.EqualError(t, err, tt.expErr)
+				assert.Nil(t, cc)
+			} else {
+				assert.NoError(t, err)
+				require.NotNil(t, cc)
+				assert.Equal(t, tt.gp, cc.shutdownGracePeriod)
+			}
+		})
+	}
+}
+
 func Test_createHTTPServerUsingBuilder(t *testing.T) {
 
 	var httpBuilderNoErrors = []error{}
@@ -83,6 +108,7 @@ func Test_createHTTPServerUsingBuilder(t *testing.T) {
 		errors.New("route builder is nil"),
 		errors.New("empty list of middlewares provided"),
 		errors.New("invalid cert or key provided"),
+		errors.New("negative or zero shutdown grace period provided"),
 	}
 
 	rb := NewRoutesBuilder().Append(NewRawRouteBuilder("/", func(http.ResponseWriter, *http.Request) {}).MethodGet())
@@ -93,6 +119,7 @@ func Test_createHTTPServerUsingBuilder(t *testing.T) {
 		p        int
 		rt       time.Duration
 		wt       time.Duration
+		gp       time.Duration
 		rb       *RoutesBuilder
 		mm       []MiddlewareFunc
 		c        string
@@ -105,6 +132,7 @@ func Test_createHTTPServerUsingBuilder(t *testing.T) {
 			p:   httpPort,
 			rt:  httpReadTimeout,
 			wt:  httpIdleTimeout,
+			gp:  shutdownGracePeriod,
 			rb:  rb,
 			mm: []MiddlewareFunc{
 				NewRecoveryMiddleware(),
@@ -120,6 +148,7 @@ func Test_createHTTPServerUsingBuilder(t *testing.T) {
 			p:        -1,
 			rt:       -10 * time.Second,
 			wt:       -20 * time.Second,
+			gp:       -15 * time.Second,
 			rb:       nil,
 			mm:       []MiddlewareFunc{},
 			c:        "",
@@ -132,7 +161,7 @@ func Test_createHTTPServerUsingBuilder(t *testing.T) {
 		t.Run(name, func(t *testing.T) {
 			gotHTTPComponent, err := NewBuilder().WithAliveCheckFunc(tc.acf).WithReadyCheckFunc(tc.rcf).
 				WithPort(tc.p).WithReadTimeout(tc.rt).WithWriteTimeout(tc.wt).WithRoutesBuilder(tc.rb).
-				WithMiddlewares(tc.mm...).WithSSL(tc.c, tc.k).Create()
+				WithMiddlewares(tc.mm...).WithSSL(tc.c, tc.k).WithShutdownGracePeriod(tc.gp).Create()
 
 			if len(tc.wantErrs) > 0 {
 				assert.EqualError(t, err, errs.Aggregate(tc.wantErrs...).Error())