search

outbrain-inc / orchestrator

MySQL replication topology manager/visualizer
Other
827 stars 168 forks source link

config.Config is not safe because of config.Reload() being used in a signal handler. #218

Open sjmudd opened 8 years ago

sjmudd commented 8 years ago

You use config.Reload() based on signal handler activity to reload the configuration.

However other parts of the code may be reading config.Config.SomeVariable and as far as I can see there is no locking to prevent this so there's a race condition which is dangerous.

I noticed this adding some code to make some of the timer configuration dynamic, picked up by a SIGHUP, where as now these values are not modified even if the configuration is reloaded and this was pointed out by a colleague.

I've not looked to resolve this as the pattern of using config.Config.SomeSetting is very heavily used and to protect against corruption is likely to be quite intrusive so would like to see how this can be addressed.

shlomi-noach commented 8 years ago

Confirmed, this makes a race condition.

dgryski commented 8 years ago

The easiest solution to this is probably to store the configuration in an https://golang.org/pkg/sync/atomic/#Value .