patternjavaMinor
Updating list of string while other threads are reading from it
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:
Now there will be threads accessing the whitelist like:
Is the implementation thread safe?
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
You can guarantee this at runtime by wrapping it in
However because you don't use any other feature of the AtomicReference other than get and set, you don't need
And check with
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.
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.