AtomicReference getAndSet not working as expected

Question

I have a class called LocalIpBlockingDenyListResponse that has two class members viz a List<CustomClass> customClassList and an int storing hashCode() of customClassList

I have atomic reference of above class defined in another class like,

private final AtomicReference<LocalIpBlockingDenyListResponse> denyListAtomicReference;

which is being initialized in the constructor like this

this.denyListAtomicReference = new AtomicReference<>(new LocalIpBlockingDenyListResponse(new ArrayList<>()));

I have this code in a method in same class thats updating atomic reference

try {
List<IpBlockingDenyListRecord> newDenyList = sonarisConsumer. getSonarisSuggestions);
List<IpBlockingDenyListRecord> validatedDenyList = validate (newDenyList); consumptionMetrics.addCount("denyListSize" , validatedDenyList.size), Unit.ONE);
setDenyListAtomicReference(validatedDenyList);
}

This is what setDenyListAtomicReference looks like

void setDenyListAtomicReference(List<IpBlockingDenyListRecords newDenyList) {
denyListAtomicReference.getAndSet(new LocalIpBlockingDenyListResponse(newDenyList));
}

But when I call getIpBlockingDenyList

public List<IpBlockingDenyListRecord> getIpBlockingDenyList() {
log.info("localDenyListSize: " + denyListAtomicReference.get() . getIpBlockingDenyList().size());
return denyListAtomicReference.get().getIpBlockingDenyList();

it returns an empty list.

I tried using set instead of getAndSet. Added bunch of log and debug statements but I am not able to understand why reference is not updating. I have confirmed that validatedDenyList holds the records as expected and is non empty.

Can someone please help me figure out what I am doing wrong?

Answer 1

Multiple AtomicReference#get calls is not atomic

it returns an empty list.

You have not included enough detail for us to diagnose this problem. However, I could guess that it may come from one huge mistake you made in your code: Calling AtomicReference#get twice.

In this block:

public List<IpBlockingDenyListRecord> getIpBlockingDenyList() {
    log.info("localDenyListSize: " + denyListAtomicReference.get().getIpBlockingDenyList().size());
    return denyListAtomicReference.get().getIpBlockingDenyList();
}

… you call denyListAtomicReference.get() twice. That pair of calls is not atomic. Consider this sequence of events:

Sequence	Thread 101 (your code above)	Thread 27 (some other imaginary code)
1	denyListAtomicReference.get() returns list x
2		denyListAtomicReference.set( y ) call changes the reference from list x to list y
3	denyListAtomicReference.get() returns list y

In this scenario you would be reporting on the size of list x while handing off an entirely different list y.

You should change that block to call get only once.

public List<IpBlockingDenyListRecord> getIpBlockingDenyList() {
    List<IpBlockingDenyListRecord> list = denyListAtomicReference.get().getIpBlockingDenyList() ;
    log.info("localDenyListSize: " + list.size());
    return list;
}

---

Don't call get if you don't want the value returned

Separate issue: Call set rather than getAndSet if you don't capture the returned value.

Your line:

denyListAtomicReference.getAndSet(new LocalIpBlockingDenyListResponse(newDenyList));

… returns the old value contained in the atomic reference before setting a new value in the atomic reference. But you do not bother to capture a reference to the old value returned. So there is no point here in "getting". Just call set of you don't care about the old value.

denyListAtomicReference.set(new LocalIpBlockingDenyListResponse(newDenyList));

---

Alternate example

Let's rewrite your code for simplicity.

I have a class called LocalIpBlockingDenyListResponse that has two class members viz a List customClassList and an int storing hashCode() of customClassList

Let's use a record for simplicity. Note that we guard our List property by ensuring it is immutable with call to List.copyOf combined with new ArrayList.

package work.basil.example.atomics;

import java.util.ArrayList;
import java.util.Collection;
import java.util.List;
import java.util.Objects;

public record People( List < String > list ,
                      int hash )
{
    public static People of ( final Collection < String > namesInput )
    {
        List < String > names = List.copyOf ( new ArrayList <> ( namesInput ) );
        return new People ( names , Objects.hashCode ( names ) );
    }

    public People
    {
        Objects.requireNonNull ( list );
        if ( Objects.hashCode ( list ) != hash ) throw new IllegalArgumentException ( "Received an invalid hash code number" );
    }
}

Assign an object of that record class to an atomic reference. And then replace that reference several times in other threads. I believe this code will result in there never being an empty list.

package work.basil.example.atomics;

import java.time.Duration;
import java.time.Instant;
import java.util.Collection;
import java.util.List;
import java.util.Objects;
import java.util.concurrent.Executors;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ThreadLocalRandom;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicReference;

public class ListRef
{
    public final AtomicReference < People > peopleRef;

    public ListRef ( People peopleArg )
    {
        Objects.requireNonNull ( peopleArg );
        this.peopleRef = new AtomicReference <> ( peopleArg );
    }

    public void replacePeople ( Collection < String > names )
    {
        People newPeople = People.of ( names );
        People oldPeople = this.peopleRef.getAndSet ( newPeople );
        String report = "Replaced " + oldPeople + " with " + newPeople + " at " + Instant.now ( );
        System.out.println ( report );  // Beware: System.out does NOT necessarily report its inputs in chronological order. Always include and study a timestamp.
    }

    public static void main ( String[] args )
    {
        People people = People.of ( List.of ( "Alice" , "Bob" ) );
        ListRef app = new ListRef ( people );
        app.demo ( );
    }

    private void demo ( )
    {
        System.out.println ( "Demo beginning at " + Instant.now ( ) );
        List < List < String > > manyNames =
                List.of (
                        List.of ( "Carol" , "Davis" ) ,
                        List.of ( "Edith" , "Frank" ) ,
                        List.of ( "Gail" , "Harold" ) ,
                        List.of ( "Irene" , "Jack" ) ,
                        List.of ( "Karen" , "Lenard" )
                );
        try ( ScheduledExecutorService ses = Executors.newScheduledThreadPool ( 3 ) )
        {
            for ( List < String > names : manyNames )
            {
                Duration d = Duration.ofSeconds ( ThreadLocalRandom.current ( ).nextInt ( 5 , 25 ) );
                ses.schedule (
                        ( ) -> this.replacePeople ( names ) ,
                        d.toSeconds ( ) ,
                        TimeUnit.SECONDS
                );
            }
        }
        System.out.println ( "Demo ending at " + Instant.now ( ) );
    }
}

When run:

Demo beginning at 2023-08-03T06:55:43.175423Z
Replaced People[list=[Alice, Bob], hash=1963929334] with People[list=[Karen, Lenard], hash=217763130] at 2023-08-03T06:55:51.215807Z
Replaced People[list=[Karen, Lenard], hash=217763130] with People[list=[Gail, Harold], hash=-2072015662] at 2023-08-03T06:55:52.185813Z
Replaced People[list=[Gail, Harold], hash=-2072015662] with People[list=[Irene, Jack], hash=-2094338259] at 2023-08-03T06:55:53.185362Z
Replaced People[list=[Irene, Jack], hash=-2094338259] with People[list=[Edith, Frank], hash=2139146613] at 2023-08-03T06:55:59.185048Z
Replaced People[list=[Edith, Frank], hash=2139146613] with People[list=[Carol, Davis], hash=2077047731] at 2023-08-03T06:56:07.182682Z
Demo ending at 2023-08-03T06:56:07.183702Z