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?
AtomicReference#get
calls is not atomicit 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;
}
get
if you don't want the value returnedSeparate 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));
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