Skip to content

Commit

Permalink
Copy iteration var into block-local to prevent clobbering spawned Gs.
Browse files Browse the repository at this point in the history
  • Loading branch information
@burke
burke committed Dec 3, 2013
1 parent 1103e8c commit e9d9675
Showing 1 changed file with 6 additions and 3 deletions.
9 changes: 6 additions & 3 deletions client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ func (client *Client) Close() error {
defer client.lock.Unlock()

for _, broker := range client.brokers {
go withRecover(func() { broker.Close() })
myBroker := broker // NB: block-local prevents clobbering
go withRecover(func() { myBroker.Close() })
}
client.brokers = nil
client.leaders = nil
Expand Down Expand Up @@ -180,7 +181,8 @@ func (client *Client) disconnectBroker(broker *Broker) {
delete(client.brokers, broker.ID())
}

go withRecover(func() { broker.Close() })
myBroker := broker // NB: block-local prevents clobbering
go withRecover(func() { myBroker.Close() })
}

func (client *Client) refreshMetadata(topics []string, retries int) error {
Expand Down Expand Up @@ -286,7 +288,8 @@ func (client *Client) update(data *MetadataResponse) ([]string, error) {
client.brokers[broker.ID()] = broker
Logger.Printf("Registered new broker #%d at %s", broker.ID(), broker.Addr())
} else if broker.Addr() != client.brokers[broker.ID()].Addr() {
go withRecover(func() { client.brokers[broker.ID()].Close() })
myBroker := client.brokers[broker.ID()] // use block-local to prevent clobbering `broker` for Gs
go withRecover(func() { myBroker.Close() })
broker.Open(client.config.ConcurrencyPerBroker)
client.brokers[broker.ID()] = broker
Logger.Printf("Replaced registered broker #%d with %s", broker.ID(), broker.Addr())
Expand Down

5 comments on commit e9d9675

@eapache
Copy link
Contributor

@eapache eapache commented on e9d9675 Dec 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't Go's closure mechanics prevent this problem already? If not there may be other places with this problem as that was an assumption I had...

@burke
Copy link
Contributor Author

@burke burke commented on e9d9675 Dec 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, it's a problem a lot of us have encountered now. It's surprising, yes? I fixed it everywhere we range over an iteration variable and spawn a goroutine, so the sarama codebase should be fine now.

@eapache
Copy link
Contributor

@eapache eapache commented on e9d9675 Dec 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found https://code.google.com/p/go-wiki/wiki/CommonMistakes which suggests parametrizing the go routine instead.

@burke
Copy link
Contributor Author

@burke burke commented on e9d9675 Dec 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's more difficult with the withRecover wrapper due to Go's lack of generics. Maybe there's some way to do it, but it wasn't immediately obvious to me.

@eapache
Copy link
Contributor

@eapache eapache commented on e9d9675 Dec 3, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is why I wanted to put the defer call inside the go routines rather than passing to a wrapper in the first place. You could do it with reflection, but that seems unnecessarily complicated.

Please sign in to comment.