Fix concurrency of VersionedIntervalTimeline

[leventov]

getAllTimelineEntries() allows accessing a collection that might be modified concurrently. See https://github.com/code-review-checklists/java-concurrency#unsafe-concurrent-iteration

[gianm]

The method is marked @VisibleForTesting, which is like a way of saying "should be private, but has to be public so tests can use it". So probably the real bug is that DruidCoordinatorLogger is using it at all. It also brings up that it would be nice to have some automation around detecting when @VisibleForTesting are used in production code.

[leventov]

@gianm it's not a bug that DruidCoordinatorLogger uses this method. @VisibleForTesting is also used to "explain" why some method is package-private rather than private or public rather than package-private. There is no annotation in Guava for methods that should be used only in test code.

It seems that the right way to express this is to add "InTest" suffix to the method name and also say in Javadoc comment that the method should be called only from test code. But we can create an annotation like "@ForTestsOnly".

[gianm]

In that case, maybe I misunderstand what @VisibleForTesting is for. Personally I've always used it to mean "would be private, but needs to be package-private or public so it can be used in tests". If it doesn't mean that, it'd be nice to add an annotation that does mean that, since I think it's useful. That way it could potentially be verified automatically.

[leventov]

I think we shouldn't add an annotation without enforcing that the annotated methods are not called from production code (that might be non-trivial). This is because in the absence of annotation when people should add "InTest" suffix to the method name, it's less likely that somebody makes a mistake and calls a method with "InTest" suffix in production code. It is too visible. Having an annotation, developers won't add "InTest" suffix to method names, and it will still be easy to mistakenly call them from production code.

[gianm]

I don't really love that solution. Adding "InTest" seems to me like it would be pretty ugly, especially in the 200+ places that we have a VisibleForTesting annotation today. And using a naming convention like this could make people mistakenly think the method is only used for testing. Lots of the methods with this annotation today are meant to be used in production, but just privately.

Maybe better to keep things the way they are, then.

[leventov]

Lots of the methods with this annotation today are meant to be used in production, but just privately.

Those methods should not have "InTest" suffix, because if they needed privately, they are used in production code. Or, we should add a separate, third annotation "OnlyUsePrivatelyOrInTests".

[leventov]

BrokerServerView.getTimeline() is another manifestation of this problem: it's an example of https://github.com/code-review-checklists/java-concurrency#concurrent-mutation-race.

[jihoonson]

@leventov thanks for finding this bug.

BrokerServerView.getTimeline() is another manifestation of this problem: it's an example of https://github.com/code-review-checklists/java-concurrency#concurrent-mutation-race.

Why is this an example of concurrent mutation race rather than unsafe concurrent iteration? Looks like all users of BrokerServerView.getTimeline() just call lookup() on it.

[leventov]

@jihoonson apparently, it's both, since one concurrency checklist item of these is a generalization of another. I didn't look at getTimeline() uses. Thanks for looking further.