HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavaMinor

Updating list of string while other threads are reading from it

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
fromreadingwhilearethreadsupdatinglistotherstring

Problem

I have a logging solution that writes down the hashes of messages. I consider some of these hashes to be safe and I want to skip logging those. I have implemented a whitelist that is a directory containing whitelisted hashes. There is a timer task that updates that list every 10 minutes.

I have tried testing it from multiple angles, but experience shows that it is near to impossible to generate the load with the diversity that I see in the wild. So could someone point out any shortcomings in my approach, only relevant parts showed:

public class MyLogger {

    private volatile AtomicReference> whitelist = new AtomicReference>();

    private void startWhitelistListener() {
        new Timer().scheduleAtFixedRate(
                new TimerTask() {
                    @Override
                    public void run() {
                        updateWhitelist();
                    }
                }, 1000, 60 * 10 * 1000
        );
    }

    private void updateWhitelist() {
        try{
            ArrayList tempList = new ArrayList();
            for (File f : scriptWhitelistDirectory.listFiles()){
                tempList.add(f.getName());
            }
            whitelist.set(tempList);
        } catch (Throwable ignore){}
    }
....
}


Now there will be threads accessing the whitelist like:

if (whitelist.get().contains(hash)) return null;


Is the implementation thread safe?

Solution

Yes if you only read the whitelist.

You can guarantee this at runtime by wrapping it in Collections.unmodifiableList while setting.

whitelist.set(Collections.unmodifiableList(tempList));


However because you don't use any other feature of the AtomicReference other than get and set, you don't need AtomicReference at all (or if you do it should be final); just volatile is enough:

private volatile List whitelist = Collections.emptyList();//empty dummy list

whitelist = Collections.unmodifiableList(tempList);


And check with if (whitelist.contains(hash)) return null;

That leads me to the list itself; if you only check whether a values is contained within the list then you should consider changing the list to a HashSet.

private volatile Set whitelist = Collections.emptySet();

Set tempList = new HashSet();
for (File f : scriptWhitelistDirectory.listFiles()){
    tempList.add(f.getName());
}
whitelist = Collections.unmodifiableSet(tempSet);

Code Snippets

whitelist.set(Collections.unmodifiableList(tempList));
private volatile List<String> whitelist = Collections.emptyList();//empty dummy list

whitelist = Collections.unmodifiableList(tempList);
private volatile Set<String> whitelist = Collections.emptySet();

Set<String> tempList = new HashSet<String>();
for (File f : scriptWhitelistDirectory.listFiles()){
    tempList.add(f.getName());
}
whitelist = Collections.unmodifiableSet(tempSet);

Context

StackExchange Code Review Q#76950, answer score: 4

Revisions (0)

No revisions yet.