search

burke / zeus

Boot any rails app in under a second.
MIT License
3.33k stars 231 forks source link

Make SlaveNode State access threadsafe #546

Closed metcalf closed 8 years ago

metcalf commented 8 years ago

Currently requestRestart accesses the State attribute of the SlaveNode's Slaves without holding a mutex. This PR makes state a private attribute with a public accessor that automatically locks before reading.

In the process I removed the stateChanged Cond which appears to be left over from an old version of the code: https://github.com/burke/zeus/blob/e20fb4ae6a510ec2c0444d991f4ae12db4111212/go/zeusmaster/slavenode.go

Do you have a standard test plan for changes to the Zeus codebase? I'm happy to try merging this into the Stripe fork first and running it here if that's the best approach but I think it'd be nice to upstream.

cc @ptarjan

metcalf commented 8 years ago

cc @antifuchs

antifuchs commented 8 years ago

Thanks for the ping, @metcalf! I'll review this next week & hopefully get it merged later.

I don't have much advice on tests, sadly - if you have one, that would be lovely. I suspect that it'll involve being able to stub the commands used to run nodes.

metcalf commented 8 years ago

I haven't tried this out locally since I didn't have a good process for building it. I would not merge as-is until one of us has had a chance to give it a smoke test.

metcalf commented 8 years ago

Rebased on master to pick up integration tests and ready for review @antifuchs!

antifuchs commented 8 years ago

Looks great to me, thanks for working on this!

👍