Azure / Azure/azure-relay-java
Suspected visibility issues in the highly mt code
- Dominant language
- Java
- Stars
- 8
- Forks
- 15
- PR merge metrics
- No merged PRs in 30d
Description
While analysing some issue we currently face (cause yet unknown), I'd like to make some notes about suspected 'visibility' issues accross this API library.
When one thread updates a field it cannot be guaranteed that another thread sees the same value unless the field is declared volatile or the writing thread and the reading thread both lock on the same lock/monitor.
With this thought in mind I looked at fields which are often updated while locking on thisLock but thisLock is not locked while reading: there's no guarantee the reader sees the latest update of the field.
Please consider this general comment and review the code + run it through standard code analysis tools.
My examples of suspicion are:
HybridConnectionListener line 521 reading:
CompletableFuture controlConnectionTask = this.controlConnection.connectAsyncTask;
[ this method seems only to exisit for the purpose of unit testing?? ]
Maybe more serious issue:
HybridConnectionListener ->ControlConnection
line 545: private boolean closeCalled;
(not volatile declared?!)
And on line 809 it's accessed outside of thisLock:
if (!webSocket.isOpen()) {
this.closeOrAbortWebSocketAsync(connectTask, webSocket.getCloseReason());
if (this.closeCalled) {
keepGoing = false;
}
At this point assuming this code is executed by another thread than the one setting this.closeCalled, it's possible that this thread does not see this.closeCalled set to true and will continue retrying to reconnect (keepGoing true). This is currently a suspicion that we have why the listener continues to reconnect when we try to close it!
A much better practise would be to use AtomicBoolean, AtomicReference or other java.concurrent objects for any 'state' that is shared between threads (looking at the code any field is a suspect with the extensive use of CompletableFuture's / multi threading!). The concurrent objects would also allow more efficient get/set and increment operations.
One last example I suspect:
HybridConnectionListener -> ControlConnection
Line 543: private int connectDelayIndex;
(not volatile, consider AtomicInteger!!)
On line 699 this int is accessed outside of a lock and its value could be non consistent , i.e. assuming the wrong connection delay:
private CompletableFuture connectAsync(Duration timeout) {
try {
this.listener.throwIfDisposed();
CompletableFuture delayTask = CompletableFutureUtil.delayAsync(RelayConstants.CONNECTION_DELAY_INTERVALS[this.connectDelayIndex], EXECUTOR);
(note that it's always updated under control of thisLock! However when reading the lock should also be taken to ensure consistent read and prevent visibility issues!)
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.