Fix deadlock in keepSafe

[robert-milan]

No description provided.

[Dieterbe]

is it safe to do un-synchronized writes and reads on the same variable? i believe with go's memory model, the answer is no.
i would keep the lock around the write (or if you want to be really fancy, do an atomic operation on an int, but that seems overkill)

[robert-milan]

Yeah, it's not safe heh. I was thinking atomic. We can't keep the write locks around it, that is what is causing the deadlock. The other goroutine will never acquire the lock, because goroutine 1 won't release it, because it is waiting for gourtine 2 to return and decrement the waitgroup.

I guess we could just restrict it to the write though, good point.

[Dieterbe]

looks good. but you can get rid of the select and just for range over the ticker channel directly

[woodsaj]

couldnt the race here be more easily solved by just removing the locks from Stop()?
I dont see any reason why the lock was originally being acquired before closing the k.closed chan and calling wg.Wait(). Stop() should only ever be called once.

[robert-milan]

couldnt the race here be more easily solved by just removing the locks from Stop()?
I dont see any reason why the lock was originally being acquired before closing the k.closed chan and calling wg.Wait(). Stop() should only ever be called once.

It should not be called twice, but I'm not as familiar with this code. The current change is safer though, otherwise if our assumption of Stop() never being called twice is wrong then we will just get a panic.

[woodsaj]

I am not really happy with this approach, i think we need to just use a simple channel that is closed when stop is called.
The main problems i have are:

1. using a "shutdown" chan is the most common way of dealing with "plugins" like this, and is how we do this in just about every other piece of code we write. It is completely acceptable for a panic to arise if "Stop()" is called multiple times (the panic will be due to calling close() on an already closed channel). There is also an expectation that "plugins" wont be used after stop() has been called.

2. This approach results in an additional delay of up to k.periodKeep between when "Stop()" is called and when the "keepClean()" goroutine exits. A quick look over the code suggests that this would be a pretty bad thing.
   https://github.com/graphite-ng/carbon-relay-ng/blob/master/destination/destination.go#L295

https://github.com/graphite-ng/carbon-relay-ng/blob/master/destination/destination.go#L304

the call to conn.clearRedo() calls c.keepSafe.Stop().

With the approach used in this PR, a delay of up to 10seconds will be added when connections drop and and reconnect.

[robert-milan]

I am not really happy with this approach, i think we need to just use a simple channel that is closed when stop is called.
The main problems i have are:

1. using a "shutdown" chan is the most common way of dealing with "plugins" like this, and is how we do this in just about every other piece of code we write. It is completely acceptable for a panic to arise if "Stop()" is called multiple times (the panic will be due to calling close() on an already closed channel). There is also an expectation that "plugins" wont be used after stop() has been called.

As long as we are fine with panics I can change it back to using a channel

2. This approach results in an additional delay of up to k.periodKeep between when "Stop()" is called and when the "keepClean()" goroutine exits. A quick look over the code suggests that this would be a pretty bad thing.

Yes, I did think about that. I wasn't sure if it would be a problem or not.

https://github.com/graphite-ng/carbon-relay-ng/blob/master/destination/destination.go#L295

https://github.com/graphite-ng/carbon-relay-ng/blob/master/destination/destination.go#L304

the call to conn.clearRedo() calls c.keepSafe.Stop().

With the approach used in this PR, a delay of up to 10seconds will be added when connections drop and and reconnect.

Fair enough