Preamble

If ServerPool isn’t immutable, none of this code is ever going to work right and cannot be salvaged. Every other line assumes that e.g. serverPool.getServerInstances().size() returns a constant value and would go completely wrong if that expression results in different values; there’s also no thread Happens-Before setup to convey any changes made. But, perhaps indeed ServerPool cannot change or isn’t expected to. Point is, the rest of this answer assumes it doesn’t change, because if it does, you’d have to start over anyway.

No, this is not ‘thread safe’.

‘thread safe’ is not well defined. So I shall define it properly here: You want your rules not to be broken; if 12 requests are made to a pool of 3 servers, each server should be asked to deal with exactly 4 requests, no more, and no less.

Here’s an example flow that breaks your rules:

1. The pool has 3 servers; they are all alive (X, Y, and Z).
2. Thread A calls getNextPeer and gets server X.
3. Thread B calls getNextPeer simultaneously and gets server Y.
4. Thread A is pre-empted for some reason.
5. Thread B ends up calling .set(Y).
6. Thread A continues and calls .set(X).
7. time passes.
8. Some code calls getNextPeer. They get.. server Y again.

So now server X has handled 1 request, server Y has handled 2, and server Z has handled 0, which breaks your rules.

Solution

Don’t call set. If a server is dead, you call .getNext again. You can only call .getOrIncrement.

You may also want to check the interaction between overflow and modulo (%); I think it works out but I’m not sure.

You can reset the counter using compareAndSet, which is safe (whereas set is not). You use cAS in a loop: “If counter is still size, set it to 0. If it is not size, tell me, and do not do anything”. And in that case, evidently some other thread did the job so you can just return. That might be simpler; keeps your index in 0-len range instead of heading to the billions over time.

In general certain operations in AtomicInteger have well defined effects if multiple threads invoke it such that invocation order does not matter: For example, if the value is ‘8’ now and 2 threads call .incrementAndGet() simultaneously, then one of the threads gets 9 and the other gets 10. It is not clear which one gets which, but it is guaranteed, one gets 9 and the other gets 10. As long as your code can deal with that (and that is usually the case), then that is fine.

But if one thread calls .set(9) and the other calls .set(10) arbitrarily, then the value afterwards is 9 or 10, you can’t tell, and your code would have to be written such that it doesn’t matter, and that’d be very weird. If it doesn’t matter why have that variable at all then?

Similarly, cAS operations tend to be ‘safe’ if used well. So, .getAndIncrement/.incrementAndGet as well as .compareAndSet tend to be the ‘safe’ tools, and .set tends to be the ‘unsafe’ tool. But thread safety is not a black and white situation. A whole codebase is or is not thread safe. It depends on what other code does with the variable. A single operation cannot be deemed ‘thread safe’ unless you define which guarantees it must provide. Which is a thing you can only do by knowing what all other code does with it.