Potential multi-threading issue in tests with timeouts

[PhillHenry]

FailOnTimeout.StatementThread.fFinished should be volatile since at least 2 threads access it: the main thread and StatementThread (which is started by main).

This is "to ensure that updates to a variable are propagated predictably to other threads" (Java Concurrency in Practice, 3.1.4, Joshua Bloch).

[Tibor17]

+1
a god point

I recommend to use volatile on fExceptionThrownByOriginalStatement as well .

I would recommend you to redesign the constructor in order to initializing the volatile variables before final in the constructor to make sure that all are properly initialized before constructor freeze. The volatile needs write a dummy operation first even if false or null by the spec:

private final Statement fStatement; private volatile boolean fFinished; private volatile Throwable fExceptionThrownByOriginalStatement;

public StatementThread(Statement statement) {
fExceptionThrownByOriginalStatement= null;
fFinished= false;
fStatement= statement;
}


[dsaff]

This is an old issue, but may be relatively easy to fix for someone looking for a chance to contribute.

[codingricky]

@dsaff is this still an issue as FailOnTimeout has variables that are final now so therefore threadsafe.

Thanks.

[kcooney]

@codingricky I think this issue was fixed, but the recent changes to FailOnTimeout may have made it not thread-safe since fThreadGroup is neither final nor volatile. I'll comment on pull request #742

[kcooney]

The remaining issues were fixed in #765