Race issue when using sarama.NewSyncProducerFromClient with a shared client

[zaa]

Versions

Sarama Version: gopkg.in/Shopify/sarama.v1 v1.10.1
Kafka Version: 0.10.0.1
Go Version: 1.7.3

Configuration

Default both for sarama and kafka

Problem Description

Our code uses shared sarama.Client instance in two goroutines. Each goroutine tries to create a sync publisher using the shared client. When we run our code with race detection enabled, we receive the following error:

WARNING: DATA RACE
Write at 0x00c4200788e0 by goroutine 61:
  vendor/gopkg.in/Shopify/sarama%2ev1.newSyncProducerFromAsyncProducer()
      /depot/src/vendor/gopkg.in/Shopify/sarama.v1/sync_producer.go:57 +0x61
  vendor/gopkg.in/Shopify/sarama%2ev1.NewSyncProducerFromClient()
      /depot/src/vendor/gopkg.in/Shopify/sarama.v1/sync_producer.go:53 +0xdf
...

Previous read at 0x00c4200788e0 by goroutine 89:
  vendor/gopkg.in/Shopify/sarama%2ev1.(*asyncProducer).returnSuccesses()
      /depot/src/vendor/gopkg.in/Shopify/sarama.v1/async_producer.go:843 +0x91
  vendor/gopkg.in/Shopify/sarama%2ev1.(*brokerProducer).handleSuccess.func1()
      /depot/src/vendor/gopkg.in/Shopify/sarama.v1/async_producer.go:738 +0x294
  vendor/gopkg.in/Shopify/sarama%2ev1.(*produceSet).eachPartition()
      /depot/src/vendor/gopkg.in/Shopify/sarama.v1/produce_set.go:108 +0x1e0
  vendor/gopkg.in/Shopify/sarama%2ev1.(*brokerProducer).handleSuccess()
      /depot/src/vendor/gopkg.in/Shopify/sarama.v1/async_producer.go:751 +0x90
  vendor/gopkg.in/Shopify/sarama%2ev1.(*brokerProducer).handleResponse()
      /depot/src/vendor/gopkg.in/Shopify/sarama.v1/async_producer.go:703 +0x2f0
  vendor/gopkg.in/Shopify/sarama%2ev1.(*brokerProducer).run()
      /depot/src/vendor/gopkg.in/Shopify/sarama.v1/async_producer.go:637 +0x1383
  vendor/gopkg.in/Shopify/sarama%2ev1.(*brokerProducer).(vendor/gopkg.in/Shopify/sarama%2ev1.run)-fm()
      /depot/src/vendor/gopkg.in/Shopify/sarama.v1/async_producer.go:536 +0x41
  vendor/gopkg.in/Shopify/sarama%2ev1.withRecover()
      /depot/src/vendor/gopkg.in/Shopify/sarama.v1/utils.go:46 +0x50

I believe the issue is caused by the fact that when NewSyncProducerFromClient creates an async producer, the producer sets its conf field to the shared client's config (see https://github.com/Shopify/sarama/blob/master/async_producer.go#L89). Then, newSyncProducerFromAsyncProducer function tries to record sync producer's specific config settings to the shared instance of the client's config (https://github.com/Shopify/sarama/blob/master/sync_producer.go#L57) and this causes the data race.

Could you please let us know if this is a bug in sarama or do we need to use the library in some special fashion (as far as I understand shared usage of a client reference is allowed by documentation).

Thank you

[eapache]

This is a bug in sarama, though a harmless one in your particular case. Since the other producer is also a sync producer and the config is shared, it will just set the config to the same values it already has so there is no risk of an actual race.

The more worrying case would be if you are sharing a client between a sync producer and an async producer, and the one clobbered the config of the other. I'm not sure what the best approach is here... I could potentially take a shallow copy of the config struct in newSyncProducerFromAsyncProducer before modifying it, or I could simply validate that the config is pre-set the appropriate way and return an error if it is not.

What would you find most sensible?

[zaa]

What would you find most sensible?

I guess the issue boils down to the following question: should a single instance of a client provide possibility of staring several producers/consumers with different versions of a config.

As far as I understand (after cursory look over async_producer.go), the current version of the code assumes that all async producers of a client would use the same config. If this invariant holds true and having several copies of a config per consumer/producer is a bad thing, then yes, the sync producer's code should just validate that both conf.Producer.Return.Successes and conf.Producer.Return.Errors are set to true, otherwise - fail.

If having several configs per a client connection is ok (I do not know internal of sarama too well to judge), then I would presume that it would be more convenient to have a per async producer copy of a config (instead of a reference) which then can by overwritten by the sync producer. Or better yet, let sync producer constructor pass configuration overrides to the async producer creator.

[eapache]

should a single instance of a client provide possibility of staring several producers/consumers with different versions of a config

No. It might make sense for some config values, but for others like Version it really does not. Since there is no obvious way to split the config values based on whether it makes sense or not, I suppose requiring the user to set the config values makes the most sense. I'll PR that.