Use RWMutex instead of defer/recover to avoid publishing to closed ch…

…annel in M3 reporter (#90)

.gitignore

@@ -30,5 +30,4 @@ _testmain.go
 
 .DS_Store
 node_modules/
-
-
+.idea/

Makefile

@@ -26,8 +26,6 @@ lint:
 	@gofmt -d -s $(PKG_FILES) 2>&1 | grep -v $(LINT_IGNORE) | tee lint.log
 	@echo "Installing test dependencies for vet..."
 	@go test -i $(PKGS)
-	@echo "Checking vet..."
-	@$(foreach dir,$(PKG_FILES),go tool vet $(dir) 2>&1 | grep -v $(LINT_IGNORE) | tee -a lint.log;)
 	@echo "Checking lint..."
 	@$(foreach dir,$(PKGS),golint $(dir) 2>&1 | grep -v $(LINT_IGNORE) | tee -a lint.log;)
 	@echo "Checking for unresolved FIXMEs..."

example/main.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

histogram.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

histogram_test.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

instrument/call.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

instrument/call_test.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

instrument/types.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

key_gen.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

m3/config.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

m3/config_test.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

m3/customtransports/buffered_read_transport.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

m3/customtransports/buffered_read_transport_test.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

m3/customtransports/m3_calc_transport.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

m3/customtransports/m3_calc_transport_test.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

m3/example/local_server.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

m3/example/m3_main.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

m3/reporter.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal
@@ -94,6 +94,7 @@ const (
 var (
 	errNoHostPorts   = errors.New("at least one entry for HostPorts is required")
 	errCommonTagSize = errors.New("common tags serialized size exceeds packet size")
+	errAlreadyClosed = errors.New("reporter already closed")
 )
 
 // Reporter is an M3 reporter.
@@ -119,9 +120,14 @@ type reporter struct {
 	bucketIDTagName string
 	bucketTagName   string
 	bucketValFmt    string
-	closeChan       chan struct{}
 
-	metCh chan sizedMetric
+	status reporterStatus
+	metCh  chan sizedMetric
+}
+
+type reporterStatus struct {
+	sync.RWMutex
+	closed bool
 }
 
 // Options is a set of options for the M3 reporter.
@@ -429,41 +435,40 @@ func (r *reporter) reportCopyMetric(
 		copy.MetricValue.Timer = t
 	}
 
-	// NB(r): This is to avoid sending on a closed channel,
-	// it's faster to actually defer/recover than acquire
-	// a read lock here to ensure we aren't closed: benchmarked
-	// this with BenchmarkTimer in reporter_benchmark_test.go
-	defer func() {
-		recover()
-	}()
-
-	select {
-	case r.metCh <- sizedMetric{copy, size}:
-	default:
+	r.status.RLock()
+	if !r.status.closed {
+		select {
+		case r.metCh <- sizedMetric{copy, size}:
+		default:
+		}
 	}
+	r.status.RUnlock()
 }
 
-// Flush implements tally.CachedStatsReporter.
+// Flush sends an empty sizedMetric to signal a flush.
 func (r *reporter) Flush() {
-	// Avoid send on a closed channel
-	defer func() {
-		recover()
-	}()
-
-	r.metCh <- sizedMetric{}
+	r.status.RLock()
+	if !r.status.closed {
+		r.metCh <- sizedMetric{}
+	}
+	r.status.RUnlock()
 }
 
 // Close waits for metrics to be flushed before closing the backend.
 func (r *reporter) Close() (err error) {
-	defer func() {
-		if r := recover(); r != nil {
-			err = fmt.Errorf("close error occurred: %v", r)
-		}
-	}()
+	r.status.Lock()
+	if r.status.closed {
+		r.status.Unlock()
+		return errAlreadyClosed
+	}
 
+	r.status.closed = true
 	close(r.metCh)
+	r.status.Unlock()
+
 	r.processors.Wait()
-	return
+
+	return nil
 }
 
 func (r *reporter) Capabilities() tally.Capabilities {

m3/reporter_benchmark_test.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal
@@ -72,15 +72,15 @@ func BenchmarkCalulateSize(b *testing.B) {
 
 func BenchmarkTimer(b *testing.B) {
 	r, _ := NewReporter(Options{
-		HostPorts:  []string{"127.0.0.1:9052"},
-		Service:    "test-service",
-		CommonTags: defaultCommonTags,
+		HostPorts:    []string{"127.0.0.1:9052"},
+		Service:      "test-service",
+		CommonTags:   defaultCommonTags,
+		MaxQueueSize: DefaultMaxQueueSize,
 	})
+
 	defer r.Close()
+
 	benchReporter := r.(*reporter)
-	benchReporter.metCh = make(chan sizedMetric, DefaultMaxQueueSize)
-	// Close the met ch to end consume metrics loop
-	defer close(benchReporter.metCh)
 
 	go func() {
 		resourcePool := benchReporter.resourcePool

m3/reporter_test.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal
@@ -193,6 +193,24 @@ func TestNewReporterErrors(t *testing.T) {
 	assert.Error(t, err)
 }
 
+// TestReporterRaceCondition checks if therem is race condition between reporter closing
+// and metric reporting, when run with race detector on, this test should pass
+func TestReporterRaceCondition(t *testing.T) {
+	r, err := NewReporter(Options{
+		HostPorts:          []string{"localhost:8888"},
+		Service:            "test-service",
+		CommonTags:         defaultCommonTags,
+		MaxQueueSize:       queueSize,
+		MaxPacketSizeBytes: maxPacketSize,
+	})
+	require.NoError(t, err)
+
+	go func() {
+		r.AllocateTimer("my-timer", nil).ReportTimer(10 * time.Millisecond)
+	}()
+	r.Close()
+}
+
 // TestReporterFinalFlush ensures the Reporter emits the last batch of metrics
 // after close
 func TestReporterFinalFlush(t *testing.T) {

m3/resource_pool.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

m3/resource_pool_test.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

m3/sanitize.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

m3/scope_test.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal
@@ -61,14 +61,12 @@ func newTestReporterScope(
 		assert.NoError(t, closer.Close())
 
 		// Ensure reporter is closed too
-		var open, readStatus bool
-		select {
-		case _, open = <-r.(*reporter).metCh:
-			readStatus = true
-		default:
-		}
-		assert.True(t, readStatus)
-		assert.False(t, open)
+		r := r.(*reporter)
+		r.status.RLock()
+		closed := r.status.closed
+		r.status.RUnlock()
+
+		assert.True(t, closed)
 	}
 }
 

m3/thrift/constants.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

m3/thrift/m3.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

m3/thrift/ttypes.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

m3/thriftudp/multitransport.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

m3/thriftudp/multitransport_test.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

m3/thriftudp/transport.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

m3/thriftudp/transport_test.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

multi/reporter.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

multi/reporter_test.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

pool.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

prometheus/config.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

prometheus/example/prometheus_main.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

prometheus/reporter.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

prometheus/reporter_test.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal

prometheus/sanitize.go

@@ -1,4 +1,4 @@
-// Copyright (c) 2018 Uber Technologies, Inc.
+// Copyright (c) 2019 Uber Technologies, Inc.
 //
 // Permission is hereby granted, free of charge, to any person obtaining a copy
 // of this software and associated documentation files (the "Software"), to deal