Fix unbounded memory allocation. (#275)

* Add bounded capacity to InMemoryQueue directly
* Fix concurrency issues in InMemoryQueue when checking queue size
* Fix QueueEventDispatcher size gauge metric
* Fix leaking goroutines in QueueEventDispatcher

Author: Mike Davis

go.sum

@@ -1,61 +1,35 @@
-github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU=
-github.com/armon/consul-api v0.0.0-20180202201655-eb2c6b5be1b6/go.mod h1:grANhF5doyWs3UAsr3K4I6qtAmlQcZDesFNEHPZAzj8=
-github.com/coreos/etcd v3.3.10+incompatible/go.mod h1:uF7uidLiAD3TWHmW31ZFd/JWoc32PjwdhPthX9715RE=
-github.com/coreos/go-etcd v2.0.0+incompatible/go.mod h1:Jez6KQU2B/sWsbdaef3ED8NzMklzPG4d5KIOhIy30Tk=
-github.com/coreos/go-semver v0.2.0/go.mod h1:nnelYz7RCh+5ahJtPPxZlU+153eP4D4r3EedlOD2RNk=
-github.com/cpuguy83/go-md2man v1.0.10/go.mod h1:SmD6nW6nTyfqj6ABTjUi3V3JVMnlJmwcJI5acqYI6dE=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
-github.com/fsnotify/fsnotify v1.4.7/go.mod h1:jwhsz4b93w/PPRr/qN1Yymfu8t87LnFCMoQvtojpjFo=
+github.com/google/gofuzz v1.0.0 h1:A8PeW59pxE9IoFRqBp37U+mSNaQoZ46F1f0f863XSXw=
 github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg=
 github.com/google/uuid v1.1.1 h1:Gkbcsh/GbpXz7lPftLA3P6TYMwjCLYm83jiFQZF/3gY=
 github.com/google/uuid v1.1.1/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
 github.com/hashicorp/errwrap v1.0.0 h1:hLrqtEDnRye3+sgx6z4qVLNuviH3MR5aQ0ykNJa/UYA=
 github.com/hashicorp/errwrap v1.0.0/go.mod h1:YH+1FKiLXxHSkmPseP+kNlulaMuP3n2brvKWEqk/Jc4=
 github.com/hashicorp/go-multierror v1.1.0 h1:B9UzwGQJehnUY1yNrnwREHc3fGbC2xefo8g4TbElacI=
 github.com/hashicorp/go-multierror v1.1.0/go.mod h1:spPvp8C1qA32ftKqdAHm4hHTbPw+vmowP0z+KUhOZdA=
-github.com/hashicorp/hcl v1.0.0/go.mod h1:E5yfLk+7swimpb2L/Alb/PJmXilQ/rhwaUYs4T20WEQ=
-github.com/inconshreveable/mousetrap v1.0.0 h1:Z8tu5sraLXCXIcARxBp/8cbvlwVa7Z1NHg9XEKhtSvM=
-github.com/inconshreveable/mousetrap v1.0.0/go.mod h1:PxqpIevigyE2G7u3NXJIT2ANytuPF1OarO4DADm73n8=
 github.com/json-iterator/go v1.1.7 h1:KfgG9LzI+pYjr4xvmz/5H4FXjokeP+rlHLhv3iH62Fo=
 github.com/json-iterator/go v1.1.7/go.mod h1:KdQUCv79m/52Kvf8AW2vK1V8akMuk1QjK/uOdHXbAo4=
-github.com/magiconair/properties v1.8.0/go.mod h1:PppfXfuXeibc/6YijjN8zIbojt8czPbwD3XqdrwzmxQ=
-github.com/mitchellh/go-homedir v1.1.0/go.mod h1:SfyaCUpYCn1Vlf4IUYiD9fPX4A5wJrkLzIz1N1q0pr0=
-github.com/mitchellh/mapstructure v1.1.2/go.mod h1:FVVH3fgwuzCH5S8UJGiWEs2h04kUh9fWfEaFds41c1Y=
 github.com/modern-go/concurrent v0.0.0-20180228061459-e0a39a4cb421 h1:ZqeYNhU3OHLH3mGKHDcjJRFFRrJa6eAM5H+CtDdOsPc=
 github.com/modern-go/concurrent v0.0.0-20180228061459-e0a39a4cb421/go.mod h1:6dJC0mAP4ikYIbvyc7fijjWJddQyLn8Ig3JB5CqoB9Q=
 github.com/modern-go/reflect2 v0.0.0-20180701023420-4b7aa43c6742 h1:Esafd1046DLDQ0W1YjYsBW+p8U2u7vzgW2SQVmlNazg=
 github.com/modern-go/reflect2 v0.0.0-20180701023420-4b7aa43c6742/go.mod h1:bx2lNnkwVCuqBIxFjflWJWanXIb3RllmbCylyMrvgv0=
-github.com/pelletier/go-toml v1.2.0/go.mod h1:5z9KED0ma1S8pY6P1sdut58dfprrGBbd/94hg7ilaic=
 github.com/pkg/errors v0.8.1 h1:iURUrRGxPUNPdy5/HRSm+Yj6okJ6UtLINN0Q9M4+h3I=
 github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
 github.com/pkg/profile v1.3.0 h1:OQIvuDgm00gWVWGTf4m4mCt6W1/0YqU7Ntg0mySWgaI=
 github.com/pkg/profile v1.3.0/go.mod h1:hJw3o1OdXxsrSjjVksARp5W95eeEaEfptyVZyv6JUPA=
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
 github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
-github.com/russross/blackfriday v1.5.2/go.mod h1:JO/DiYxRf+HjHt06OyowR9PTA263kcR/rfWxYHBV53g=
-github.com/spf13/afero v1.1.2/go.mod h1:j4pytiNVoe2o6bmDsKpLACNPDBIoEAkihy7loJ1B0CQ=
-github.com/spf13/cast v1.3.0/go.mod h1:Qx5cxh0v+4UWYiBimWS+eyWzqEqokIECu5etghLkUJE=
-github.com/spf13/jwalterweatherman v1.0.0/go.mod h1:cQK4TGJAtQXfYWX+Ddv3mKDzgVb68N+wFjFa4jdeBTo=
-github.com/spf13/pflag v1.0.3 h1:zPAT6CGy6wXeQ7NtTnaTerfKOsV6V6F8agHXFiazDkg=
-github.com/spf13/pflag v1.0.3/go.mod h1:DYY7MBk1bdzusC3SYhjObp+wFpr4gzcvqqNjLnInEg4=
-github.com/spf13/viper v1.3.2/go.mod h1:ZiWeW+zYFKm7srdB9IoDzzZXaJaI5eL9QjNiN/DMA2s=
 github.com/stretchr/objx v0.1.0 h1:4G4v2dO3VZwixGIRoQ5Lfboy6nUhCyYzaqnIAPPhYs4=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
-github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
 github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
 github.com/stretchr/testify v1.4.0 h1:2E4SXV/wtOkTonXsotYi4li6zVWxYlZuYNCXe9XRJyk=
 github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4=
 github.com/twmb/murmur3 v1.0.0 h1:MLMwMEQRKsu94uJnoveYjjHmcLwI3HNcWXP4LJuNe3I=
 github.com/twmb/murmur3 v1.0.0/go.mod h1:5Y5m8Y8WIyucaICVP+Aep5C8ydggjEuRQHDq1icoOYo=
-github.com/ugorji/go/codec v0.0.0-20181204163529-d75b2dcb6bc8/go.mod h1:VFNgLljTbGfSG7qAOspJ7OScBnGdDN/yBr0sguwnwf0=
-github.com/xordataexchange/crypt v0.0.3-0.20170626215501-b2862e3d0a77/go.mod h1:aYKd//L2LvnjZzWKhF00oedf4jCCReLcmhLdhm1A27Q=
-golang.org/x/crypto v0.0.0-20181203042331-505ab145d0a9/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4=
 golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e h1:vcxGaoTs7kV8m5Np9uUNQin4BrLOthgV7252N8V+FwY=
 golang.org/x/sync v0.0.0-20190911185100-cd5d95a43a6e/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
-golang.org/x/sys v0.0.0-20181205085412-a5c9d58dba9a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
-golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/yaml.v2 v2.2.2 h1:ZCJp+EgiOT7lHqUV2J862kp8Qj64Jo6az82+3Td9dZw=

pkg/event/dispatcher.go

@@ -20,14 +20,16 @@ package event
 import (
 	"fmt"
 	"net/http"
-	"sync"
 	"time"
 
+	"golang.org/x/sync/semaphore"
+
 	"github.com/optimizely/go-sdk/pkg/logging"
 	"github.com/optimizely/go-sdk/pkg/metrics"
 	"github.com/optimizely/go-sdk/pkg/utils"
 )
 
+const maxWorkers = int64(1)
 const maxRetries = 3
 const defaultQueueSize = 1000
 const sleepTime = 1 * time.Second
@@ -40,7 +42,7 @@ type Dispatcher interface {
 // httpEventDispatcher is the HTTP implementation of the Dispatcher interface
 type httpEventDispatcher struct {
 	requester *utils.HTTPRequester
-	logger logging.OptimizelyLogProducer
+	logger    logging.OptimizelyLogProducer
 }
 
 // DispatchEvent dispatches event with callback
@@ -79,39 +81,38 @@ func NewHTTPEventDispatcher(sdkKey string, requester *utils.HTTPRequester, logge
 
 // QueueEventDispatcher is a queued version of the event Dispatcher that queues, returns success, and dispatches events in the background
 type QueueEventDispatcher struct {
-	eventQueue     Queue
-	eventFlushLock sync.Mutex
-	Dispatcher     Dispatcher
-	logger         logging.OptimizelyLogProducer
+	eventQueue Queue
+	processing *semaphore.Weighted
+	Dispatcher Dispatcher
+	logger     logging.OptimizelyLogProducer
 
 	// metrics
-	queueSize         metrics.Gauge
-	sucessFlush       metrics.Counter
-	failFlushCounter  metrics.Counter
-	retryFlushCounter metrics.Counter
+	queueSizeGauge     metrics.Gauge
+	sucessFlushCounter metrics.Counter
+	failFlushCounter   metrics.Counter
+	retryFlushCounter  metrics.Counter
 }
 
 // DispatchEvent queues event with callback and calls flush in a go routine.
 func (ed *QueueEventDispatcher) DispatchEvent(event LogEvent) (bool, error) {
 	ed.eventQueue.Add(event)
-	go func() {
-		ed.flushEvents()
-	}()
+	go ed.flushEvents()
 	return true, nil
 }
 
 // flush the events
 func (ed *QueueEventDispatcher) flushEvents() {
 
-	ed.eventFlushLock.Lock()
-
-	defer func() {
-		ed.eventFlushLock.Unlock()
-	}()
+	// Limit flushing to a single worker
+	if !ed.processing.TryAcquire(1) {
+		return
+	}
+	defer ed.processing.Release(1)
 
 	retryCount := 0
-	ed.queueSize.Set(float64(ed.eventQueue.Size()))
-	for ed.eventQueue.Size() > 0 {
+	queueSize := ed.eventQueue.Size()
+	for ; queueSize > 0; queueSize = ed.eventQueue.Size() {
+		ed.queueSizeGauge.Set(float64(queueSize))
 		if retryCount > maxRetries {
 			ed.logger.Error(fmt.Sprintf("event failed to send %d times. It will retry on next event sent", maxRetries), nil)
 			ed.failFlushCounter.Add(1)
@@ -136,10 +137,10 @@ func (ed *QueueEventDispatcher) flushEvents() {
 
 		if err == nil {
 			if success {
-				ed.logger.Debug(fmt.Sprintf("Dispatched log event %+v", event))
+				ed.logger.Debug("dispatch log event succeeded")
 				ed.eventQueue.Remove(1)
 				retryCount = 0
-				ed.sucessFlush.Add(1)
+				ed.sucessFlushCounter.Add(1)
 			} else {
 				ed.logger.Warning("dispatch event failed")
 				// we failed.  Sleep some seconds and try again.
@@ -159,7 +160,7 @@ func (ed *QueueEventDispatcher) flushEvents() {
 			ed.retryFlushCounter.Add(1)
 		}
 	}
-	ed.queueSize.Set(float64(ed.eventQueue.Size()))
+	ed.queueSizeGauge.Set(float64(queueSize))
 }
 
 // NewQueueEventDispatcher creates a Dispatcher that queues in memory and then sends via go routine.
@@ -172,13 +173,15 @@ func NewQueueEventDispatcher(sdkKey string, metricsRegistry metrics.Registry) *Q
 		dispatcherMetricsRegistry = metrics.NewNoopRegistry() // protective code to set
 	}
 
+	logger := logging.GetLogger(sdkKey, "QueueEventDispatcher")
 	return &QueueEventDispatcher{
-		eventQueue:        NewInMemoryQueue(defaultQueueSize),
-		Dispatcher:        NewHTTPEventDispatcher(sdkKey, nil, nil),
-		queueSize:         dispatcherMetricsRegistry.GetGauge(metrics.DispatcherQueueSize),
-		retryFlushCounter: dispatcherMetricsRegistry.GetCounter(metrics.DispatcherRetryFlush),
-		failFlushCounter:  dispatcherMetricsRegistry.GetCounter(metrics.DispatcherFailedFlush),
-		sucessFlush:       dispatcherMetricsRegistry.GetCounter(metrics.DispatcherSuccessFlush),
-		logger:            logging.GetLogger(sdkKey, "QueueEventDispatcher"),
+		eventQueue:         NewInMemoryQueueWithLogger(defaultQueueSize, logger),
+		Dispatcher:         NewHTTPEventDispatcher(sdkKey, nil, nil),
+		queueSizeGauge:     dispatcherMetricsRegistry.GetGauge(metrics.DispatcherQueueSize),
+		retryFlushCounter:  dispatcherMetricsRegistry.GetCounter(metrics.DispatcherRetryFlush),
+		failFlushCounter:   dispatcherMetricsRegistry.GetCounter(metrics.DispatcherFailedFlush),
+		sucessFlushCounter: dispatcherMetricsRegistry.GetCounter(metrics.DispatcherSuccessFlush),
+		logger:             logger,
+		processing:         semaphore.NewWeighted(maxWorkers),
 	}
 }

pkg/event/processor.go

@@ -166,7 +166,7 @@ func NewBatchEventProcessor(options ...BPOptionConfig) *BatchEventProcessor {
 	}
 
 	if p.Q == nil {
-		p.Q = NewInMemoryQueue(p.MaxQueueSize)
+		p.Q = NewInMemoryQueueWithLogger(p.MaxQueueSize, p.logger)
 	}
 
 	if p.EventDispatcher == nil {

pkg/event/processor_test.go

@@ -21,12 +21,14 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"github.com/optimizely/go-sdk/pkg/logging"
-	"github.com/optimizely/go-sdk/pkg/utils"
-	"github.com/stretchr/testify/assert"
 	"math"
 	"testing"
 	"time"
+
+	"github.com/stretchr/testify/assert"
+
+	"github.com/optimizely/go-sdk/pkg/logging"
+	"github.com/optimizely/go-sdk/pkg/utils"
 )
 
 type CountingDispatcher struct {
@@ -538,7 +540,7 @@ func BenchmarkProcessor(b *testing.B) {
 	}
 
 	for _, merge := range merges {
-		for i := 1.; i <= 5; i++ {
+		for i := 4.; i <= 7; i++ {
 			qs := int(math.Pow(10, i))
 			for j := 1; j <= 6; j++ {
 				bs := 10 * j

pkg/event/queue.go

@@ -19,60 +19,85 @@ package event
 
 import (
 	"sync"
+
+	"github.com/optimizely/go-sdk/pkg/logging"
 )
 
 // Queue represents a queue
 type Queue interface {
-	Add(item interface{})
+	Add(item interface{}) // TODO Should return a bool
 	Remove(count int) []interface{}
 	Get(count int) []interface{}
 	Size() int
 }
 
 // InMemoryQueue represents a in-memory queue
 type InMemoryQueue struct {
-	Queue []interface{}
-	Mux   sync.Mutex
+	logger  logging.OptimizelyLogProducer
+	MaxSize int
+	Queue   []interface{}
+	Mux     sync.Mutex
 }
 
 // Get returns queue for given count size
-func (i *InMemoryQueue) Get(count int) []interface{} {
-	if i.Size() < count {
-		count = i.Size()
-	}
-	i.Mux.Lock()
-	defer i.Mux.Unlock()
-	return i.Queue[:count]
+func (q *InMemoryQueue) Get(count int) []interface{} {
+	q.Mux.Lock()
+	defer q.Mux.Unlock()
+
+	count = q.getSafeCount(count)
+	return q.Queue[:count]
 }
 
 // Add appends item to queue
-func (i *InMemoryQueue) Add(item interface{}) {
-	i.Mux.Lock()
-	i.Queue = append(i.Queue, item)
-	i.Mux.Unlock()
+func (q *InMemoryQueue) Add(item interface{}) {
+	q.Mux.Lock()
+	defer q.Mux.Unlock()
+
+	if len(q.Queue) >= q.MaxSize {
+		q.logger.Warning("MaxQueueSize has been met. Discarding event")
+		return
+	}
+
+	q.Queue = append(q.Queue, item)
 }
 
 // Remove removes item from queue and returns elements slice
-func (i *InMemoryQueue) Remove(count int) []interface{} {
-	if i.Size() < count {
-		count = i.Size()
-	}
-	i.Mux.Lock()
-	defer i.Mux.Unlock()
-	elem := i.Queue[:count]
-	i.Queue = i.Queue[count:]
+func (q *InMemoryQueue) Remove(count int) []interface{} {
+	q.Mux.Lock()
+	defer q.Mux.Unlock()
+
+	count = q.getSafeCount(count)
+	elem := q.Queue[:count]
+	q.Queue = q.Queue[count:]
 	return elem
 }
 
+func (q *InMemoryQueue) getSafeCount(count int) int {
+	if size := len(q.Queue); size < count {
+		return size
+	}
+
+	return count
+}
+
 // Size returns size of queue
-func (i *InMemoryQueue) Size() int {
-	i.Mux.Lock()
-	defer i.Mux.Unlock()
-	return len(i.Queue)
+func (q *InMemoryQueue) Size() int {
+	q.Mux.Lock()
+	defer q.Mux.Unlock()
+	return len(q.Queue)
 }
 
 // NewInMemoryQueue returns new InMemoryQueue with given queueSize
 func NewInMemoryQueue(queueSize int) Queue {
-	i := &InMemoryQueue{Queue: make([]interface{}, 0, queueSize)}
-	return i
+	logger := logging.GetLogger("", "InMemoryQueue")
+	return NewInMemoryQueueWithLogger(queueSize, logger)
+}
+
+// NewInMemoryQueueWithLogger returns new InMemoryQueue with given queueSize and logger
+func NewInMemoryQueueWithLogger(queueSize int, logger logging.OptimizelyLogProducer) Queue {
+	return &InMemoryQueue{
+		logger:  logger,
+		MaxSize: queueSize,
+		Queue:   make([]interface{}, 0, queueSize),
+	}
 }