javaconcurrencyatomicreference

AtomicReference getAndSet not working as expected


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?


Solution

  • 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