Netflix/curator — pull request #78 by bbeck (2012-05-31 03:35:33+00:00)
Title: TestingZooKeeperMain updates

There's a race condition inside of `TestingZooKeeperMain` that causes it in certain scenarios to throw a `NullPointerException`. This happens when you start and then stop the server very quickly. I have several test cases that this happens in and I end up getting about 1 failure every 100 runs due to this race condition.

There's a very simple reproduction scenario that demonstrates this problem after a minute or two:
```java
while(true) {
TestingServer server = new TestingServer();
server.close();
server.stop();
}
```

Run this for awhile and you'll eventually see a `NullPointerException`. It helps to cause the failure more quickly if you load your machine down with some processes that consume massive amounts of CPU (`python -c "while True: pass"` works great for this). The goal is to make the 100ms sleep not sufficient, and to start to close the server before it's been started.

The underlying problem is in `TestingZooKeeperMain.runFromConfig`, but unfortunately the base class `ZooKeeperMain` doesn't give you the tools to be able to fix it. The problem is the latch in `runFromConfig` is triggered right before calling `super.runFromConfig`. Ideally we'd like to wait until after `super.runFromConfig` has started the server to trigger the latch. Unfortunately the ZooKeeper developers decided it would be okay to take over the thread calling `runFromConfig` and use it for the ZooKeeper server, so control never gets passed back to `TestingZooKeeperMain` to trigger the latch. So the best that can be done is what is already in the code that triggers the latch right before the call into `super.runFromConfig`.

What I've done in this change is to use some more reflection to get a handle to the underlying `ZooKeeperServer` object. Once you have that object, you can ask it whether or not it's started already. If it isn't, then you want wait on it until it notifies you that it's been started. Unfortunately this requires a bit of traversal through several fields so the code is a little bit messy with reflection and wait states -- most of the fields that we care about start off as null from the constructor, but eventually are initialized to non-null as the server starts up. So there are a couple of busy loops waiting from those fields to be set. I'm not really sure if there's another, possibly better, way to do this. The good news is that I was able to remove the 100ms sleep during startup and the `TestingZooKeeperMain` starts up 50-100x faster now. It also never seems to fail the above reproduction scenario after millions of iterations with a lot of heavy load on the machine.

I'd be really appreciative if you'd be willing to work with me to get this change merged in, as it would eliminate a large source of random test failures in my continuous build, plus make things run a little bit faster.