current community

your communities

more stack exchange communities

Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 1 down vote favorite

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<List<String>> whitelist = new AtomicReference<List<String>>();

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

    private void updateWhitelist() {
        try{
            ArrayList<String> tempList = new ArrayList<String>();
            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?

share|improve this question

2 Answers 2

up vote 2 down vote accepted

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<String> 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<String> whitelist = Collections.emptySet();

Set<String> tempList = new HashSet<String>();
for (File f : scriptWhitelistDirectory.listFiles()){
    tempList.add(f.getName());
}
whitelist = Collections.unmodifiableSet(tempSet);
share|improve this answer
up vote 2 down vote

Your use of volatile is misguided, and it does nothing for you, in this situation. The fact that you have used volatile here indicates that you don't understand what it does, and why you may need it.

In general, with the availability of the java.util.concurrent.*, and traditional synchronize, there is no real reason to use volatile at all. volatile is a hard-to-see memory management technique that immediately rings alarm bells for me.

while ratchet freak's suggested implementation will work fine, I strongly advise against using volatile simply because it is too hard to see.

In your case, you never change the whitelist, so there is no purpose to the volatile. Sure, you change the contents inside the whitelist, but that's different. Further, your whitelist should be final, which would be better practice, and helps with concurrency too.

private final AtomicReference<List<String>> whitelist = new AtomicReference<>();

Note above how I use the <> 'diamond operator'. Using the full generic type on the right-hand- side of an assignment is no longer needed.

The Atomic* classes ensure that the memory model for get/set operations are correctly ordered, and thread safe. As a result, the way you get and set the List in that is currently just fine.

As ratchet freak indicates, you should probably be using a Set, and not a List. A set offers constant lookup times as your set size increases.

If it were me, I would have something like:

private final AtomicReference<Set<String>> whitelist = new AtomicReference<>(Collections.emptySet());

private void updateWhitelist() {
    try{
        Set<String> tempSet = new HashSet<String>();
        for (File f : scriptWhitelistDirectory.listFiles()){
            tempList.add(f.getName());
        }
        whitelist.set(Collections.unmodifiableSet(tempList));
    } catch (Throwable neverIgnore){
        Logger.warn("Unable to read files from " + scriptWhitelistDirectory, neverIgnore);
    }
}

Note how I handle the exception too, even if it is just to log it somewhere.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.