Deadlock in metrics

[yukw777]

There's a deadlock in how the go client handles metrics data, specifically when it handles the "sent" event. There are other issues in the codebase that were uncovered during debugging, so I'll list all of them here for now, but I'm happy to make a separate issue for each if necessary.

The deadlock happens when you don't pass in an "optional" event listener (WithListener) when initializing the client. The unbuffered sent channel receives a value

unleash-client-go/metrics.go

Line 186 in c586a9e

m.sent <- payload

, but b/c the event listener is nil, the receiver is never started

unleash-client-go/client.go

Lines 97 to 110 in c586a9e

	if uc.options.listener != nil {
	if eListener, ok := uc.options.listener.(ErrorListener); ok {
	uc.errorListener = eListener
	}
	if rListener, ok := uc.options.listener.(RepositoryListener); ok {
	uc.repositoryListener = rListener
	}
	if mListener, ok := uc.options.listener.(MetricListener); ok {
	uc.metricsListener = mListener
	}
	defer func() {
	go uc.sync()
	}()
	}

, so the next time a payload is sent to the sent channel, it blocks.

There are some other issues related to this bug.

1. WithDisableMetrics doesn't work, b/c the value doesn't get passed into metrics from the clinet.
   

   unleash-client-go/client.go

   Lines 163 to 171 in c586a9e

   	metricsOptions{
   	appName: uc.options.appName,
   	instanceId: uc.options.instanceId,
   	strategies: strategyNames,
   	metricsInterval: uc.options.metricsInterval,
   	url: *parsedUrl,
   	httpClient: uc.options.httpClient,
   	customHeaders: uc.options.customHeaders,
   	},

2. If you pass in the value down to metrics, you get a nil pointer exception b/c the timer is never initialized.
   

   unleash-client-go/metrics.go

   Lines 95 to 99 in c586a9e

   	if m.options.disableMetrics {
   	return
   	}
   	m.timer = time.NewTimer(m.options.metricsInterval)

3. Setting metrics interval to 0 doesn't work either since it now blocks on other channels.
   

   unleash-client-go/metrics.go

   Lines 80 to 84 in c586a9e

   	if m.options.metricsInterval > 0 {
   	m.startTimer()
   	m.registerInstance()
   	go m.sync()
   	}

4. Metrics is too tightly coupled to the client. Proper dependency inversion would help here.

The only true workaround that works right now is to set a listener, even if it's a noop.

Happy to help out in anyway I can. Thanks!

[jrbarron]

@yukw777 Thanks for the report. This deadlock issue is known and documented here (last sentence under "Basics"):

https://godoc.org/github.com/Unleash/unleash-client-go#hdr-Basics

It was a side effect of the API design where I wanted to force people to use a listener so as not to ignore errors which as you know is bad practice in Go. In retrospect, this may have been a mistake so I have contemplated changing it, perhaps in the v3 branch.

The additional bugs you mention there certainly sound valid so I'll try to take a look at those when I get some time. I will happily accept a PR for these too if you have something ready. I've also starting writing some unit tests for the metrics struct now so hopefully that will catch some of these issues.

Thanks for the great report!

[jrbarron]

The bugs relating to disabling metrics and also the deadlocks (except for the nil Listener which is documented) should now be fixed so I'm closing this.