Azure / Azure/azure-relay-java

Suspected visibility issues in the highly mt code

Open
#65 2 comments 0 reactions 2 assignees Claimed by @kalkeea View on GitHub
Relay
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.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.