6
\$\begingroup\$

I am working on a project in which I construct a URL with a valid hostname (but not a blocked hostname) and then execute that URL using RestTemplate from my main application thread. I also have a single background thread in my application which parses the data from the URL and extracts the block list of hostnames from it.

If any block list of hostnames is present, then I won't make a call to that hostname from the main thread and I will try making a call to another hostname. By block list, I mean whenever any server is down, its hostname is on the block list.

Here is my background thread code. It will get the data from my service URL and keep on running every 10 minutes once my application has started up. It will then parse the data coming from the URL and store it in a ClientData class variable.

public class TempScheduler {

    // .. scheduledexecutors service code to start the background thread

    // call the service and get the data and then parse 
    // the response.
    private void callServiceURL() {
        String url = "url";
        RestTemplate restTemplate = new RestTemplate();
        String response = restTemplate.getForObject(url, String.class);
        parseResponse(response);

    }

    // parse the response and store it in a variable
    private void parseResponse(String response) {
        //...       

        // get the block list of hostnames
        Map<String, List<String>> coloExceptionList = gson.fromJson(response.split("blocklist=")[1], Map.class);
        List<String> blockList = new ArrayList<String>();
        for(Map.Entry<String, List<String>> entry : coloExceptionList.entrySet()) {
            for(String hosts : entry.getValue()) {
                blockList.add(hosts);
            }
        }

        // store the block list of hostnames which I am not supposed to make a call
        // from my main application
        ClientData.replaceBlockedHosts(blockList);
    }
}

Here is my ClientData class. Is it thread-safe? The replaceBlockedHosts method will only be called by a background thread meaning only one writer. But isHostBlocked will be called by main application threads multiple times to check whether the hostname is blocked or not. And also blockHost method might be called from catch block multiple times to add the down host in the blockedHosts list so I need to make sure all the read threads can see the consistent data.

public class ClientData {

    // do I need this AtomicReference with ConcurrentHashMap?
    private static final AtomicReference<ConcurrentHashMap<String, String>> blockedHosts = 
            new AtomicReference<ConcurrentHashMap<String, String>>(new ConcurrentHashMap<String, String>());

    public static boolean isHostBlocked(String hostName) {
        return blockedHosts.get().containsKey(hostName);
    }

    public static void blockHost(String hostName) {
        blockedHosts.get().put(hostName, hostName);
    }

    public static void unblockHost(String hostName) {
        blockedHosts.get().remove(hostName);
    }

    public static void replaceBlockedHosts(List<String> hostNames) {
        ConcurrentHashMap<String, String> newBlockedHosts = new ConcurrentHashMap<>();
        for (String hostName : hostNames) {
            newBlockedHosts.put(hostName, hostName);
        }
        blockedHosts.set(newBlockedHosts);
    }
}

And below is my main application thread code in which I am finding all the hostnames on which I can make a call and then iterate the hostnames list to make a call.

If that hostname is null or in the block list category, then I won't make a call to that particular hostname and will try next hostname in the list.

@Override
public DataResponse call() throws Exception {

    List<String> hostnames = new LinkedList<String>();

    // .. some separate code here to populate the hostnames list, not from ClientData class

    for (String hostname : hostnames) {     

        // If host name is null or host name is in block list category, skip sending request to this host
        if (hostname == null || ClientData.isHostBlocked(hostname)) {
            continue;
        }

        try {
            String url = generateURL(hostname);

            response = restTemplate.getForObject(url, String.class);

            break;
        } catch (RestClientException ex) {
            // does this call will be thread safe for blockHost method in ClientData class?
            ClientData.blockHost(hostname);
        }
    }
}

I don't need to make a call to the hostname whenever it is down from the main thread. And my background thread gets these detail from one of my service, whenever any server is down, it will have the list of hostnames and whenever they are up, that list will get updated.

Also, whenever any RestClientException is being thrown, I will add that hostname in the blockedHosts concurrentmap as well by updating since my background thread is running every 10 minutes so that map won't have this hostname until 10 minutes is done. And whenever this server came back up, my background will update this list automatically.

The Callable code will be called at a rate of 1000 requests per second, so it has to be fast. I want to have atomic operations so that all the main threads should see consistent data from isHostBlocked method. Also, is the ClientData.blockHost(hostname) method thread-safe?

Does my ClientData class looks right with the way I am using AtomicReference or not meaning is it thread safe?

\$\endgroup\$
4
+50
\$\begingroup\$

As I've stated in my previous answer, the first thing you must decide is "What level of atomicity does this application require?" I don't mean simple thread-safety, which the above code provides by not throwing exceptions or behaving in bizarre ways.

Here are two examples where atomicity is violated:

The first case, as Totò points out, can happen when thread T1 starts adding a host to the block list while thread T2 inspects the block list. T2 may start after T1 and yet not see the new host in the list. Note that this applies equally to unblockHost.

T1                                   T2

detect down host "foo.com"
blockHost("foo.com")
get() -> map
                                     isHostBlocked("foo.com")
                                     get() -> map
                                     map.contains("foo.com") -> false
map.put("foo.com", "foo.com")

Is this a serious problem or merely unfortunate but acceptable? You can solve this by making every method accessing the map synchronized. You'll cause more contention and decrease throughput, but the above will be impossible.

Is it worth it? Compare the above sequence with this equally possible sequence that will not be solved with synchronization:

T1                                   T2

detect down host "foo.com"
                                     isHostBlocked("foo.com")
                                     get() -> map
                                     map.contains("foo.com") -> false
blockHost("foo.com")
get() -> map
map.put("foo.com", "foo.com")

What have you solved then? T2 is still allowed to access a host that T1 has already determined should be blocked. You have to weigh the synchronization penalty (on every access) against the cost of trying to access a host that is about to be blocked--something that is unlikely to happen in the extreme given the small number of operations between them.


The second case is a bit worse: T1 starts adding a host to the block list while thread T2 replaces the block list entirely, causing T1's host to get lost in the shuffle.

T1                                   T2

detect down host "foo.com"
blockHost("foo.com")
get() -> map
                                     receive new block list ["bar.com"]
                                     replaceBlockedHosts(["bar.com"])
                                     set(["bar.com"])
map.put("foo.com", "foo.com")

...

isHostBlocked("foo.com") -> false

Granted, T2 is replacing the block list; if T1 finishes before T2 starts, the added host will be removed as soon as T2 finishes. But if the process that gives T2 the new block list completes before T1 detects the bad host, you probably want the new host added to the list.

T1                                   T2

                                     receive new block list ["bar.com"]
detect down host "foo.com"
blockHost("foo.com")
get() -> map
map.put("foo.com", "foo.com")
                                     replaceBlockedHosts(["bar.com"])
                                     set(["bar.com"])

...

isHostBlocked("foo.com") -> false

Is this a serious problem or merely unfortunate but acceptable? Again, you can synchronize every method and pay the penalty on every access to solve an otherwise acceptable problem that might happen once or twice a year. You must weight the costs against each other as there is no absolutely correct decision. As usual, it depends. :)


If these unlikely issues are acceptable, I would revert back to one of the previous answers I gave by removing the AtomicReference and rewrite replaceBlockedHosts like so

public static void replaceBlockedHosts(List<String> hostNames) {
    blockedHosts.clear();
    for (String hostName : hostNames) {
        blockHost(hostName);
    }
}

The advantage is clearer code that uses a single synchronization method: the ConcurrentHashMap. The disadvantage is that you further decrease the atomicity of replaceBlockedHosts. It's now possible for some hosts in a new list to be seen as blocked while others are not, but this can only happen in the short time it takes to add a handful of strings to a map.

\$\endgroup\$
  • \$\begingroup\$ Thanks once again. In this case, replaceBlockedHosts is only called by a single background thread which is running once my application is started up. It won't be called by main reader threads. I guess these things are avoidable for me whatever scenarios you have described since it takes very short time to add strings to map so eventually all threads will have consistent data after some time, it is just that for short period of time I guess, inconsistencies will be happening, Right? In any case, what is the advantage of this solution you gave as compared to AtomicReference one? \$\endgroup\$ – arsenal May 26 '14 at 22:58
  • 1
    \$\begingroup\$ By "short period of time" you're talking about milliseconds in the worst case. There are two advantages to dropping the AtomicReference: simpler code (most important) and one less synchronization point (ever so slightly faster, maybe nanoseconds). Synchronizing every method and using a simple HashSet is the simplest solution and provides guaranteed full atomicity, but it has the biggest performance impact of them all by serializing all access to the block list. All are valid solutions; you must pick one based on their tradeoffs. \$\endgroup\$ – David Harkness May 26 '14 at 23:22
  • \$\begingroup\$ Hope you are doing good. I have this solution running in production for a while. I am seeing pretty strange things. First of all it works fine if everything is running smooth, meaning if all the hostnames are up and no servers are in block list, then my system works very well and there are no client timeouts. But if let's say one server is down which I was suppose to make a call, then I start seeing lot of client timeouts in my above code. And I am not sure why this is happening? \$\endgroup\$ – arsenal Aug 30 '14 at 5:26
  • \$\begingroup\$ ... In general, hostnames linkedlist gets decided basis on the user id being passed. So multiple threads might make a call to the same server depending on the user id being passed. Can you think of what could be reason? \$\endgroup\$ – arsenal Aug 30 '14 at 5:27
  • \$\begingroup\$ @Webby It doesn't seem likely that a threading issue would cause network timeouts calling external servers. Post the code and exception to Stack Overflow to get more exposure and address the problem. \$\endgroup\$ – David Harkness Aug 31 '14 at 7:49
6
\$\begingroup\$

Even if in 1 line

public static void blockHost(String hostName) {
        blockedHosts.get().put(hostName, hostName);
    }

effectively consists of 2 operations, so if you want your methods to be atomic you still need synchronization. You could have an interleaving like:

T1 invokes blockHost("123") and executes get()
T2 invokes isHostBlocked("123"), does executes get() and containsKey() and returns false T1 does put(hostName, hostName);

the result of T1 is wrong because blockHost was invoked before.

I think that you need synchronization, and if you do you can probably avoid using AtomicReference

Off topic - I don't like static things... see this question on SO

\$\endgroup\$
  • \$\begingroup\$ While this interleaving is possible, it's probably fine to return false. It's no worse than T1 deciding that 123 should be blocked but T2 calling isHostBlocked before T1 can call blockHost. The real error is if T2 calls replaceBlockedHosts with the same interleaving you have above. T1 will add 123 to the old set of hosts and be lost. \$\endgroup\$ – David Harkness May 24 '14 at 19:42
  • \$\begingroup\$ the requirement says that the methods have to be atomic, both the interleaving will have wrong results \$\endgroup\$ – Totò May 25 '14 at 10:33
1
\$\begingroup\$

In general, I would suggest that you are approaching this problem with the wrong object model. Your model is:

Threads each get a 'target' by querying a thread-safe ClientData instance. If their target does not work the threads report back to the ClientData instance, and that updates itself.

This splits the responsibility for managing the 'valid' threads in to two places, and it breaks the single-responsibility SOLID principle.

Your model should instead have a callback mechanism, something like:

public interface URLGenerator {
    public String generateForHost(String hostname);
}

Then, your "Calling" code (which appears to be a Callable<DataResponse> ) should also implement the interface URLGenerator ... something like:

private static class MyCaller implements Callable<DataResponse>, URLGenerator {
    public String generateForHost(String hostname) {
        return generateURL(hostname);
    }


    public DataResponse call() throws Exception { // <-- throws Exception ... really?
        return URL_RETRIEVER.getDataResponse(this, restTemplate, String.class);
    }
}

Now, with the above code, you are giving this URL_RETRIEVER the details it needs to get the data, and you don't care how it does it. The fact that it is on multiple hosts, with block-lists, or whatever is not the point.

That URL_RETRIEVER is what your code should implement, and, my guess, is it will look something like:

import java.util.ArrayList;
import java.util.Deque;
import java.util.HashSet;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Set;


public class URLRetriever {

    private final Runnable backgroundUpdater = new Runnable () {
        @Override
        public void run() {
            updateHostList();
        }
    };

    private final Set<String> availableHosts = new HashSet<>();
    private final Deque<String> workingHosts = new LinkedList<>();

    // call the service and get the data and then parse 
    // the response.
    private final void updateHostList() {
        synchronized (backgroundUpdater) {
            // only background update at a time
            String url = "url";
            RestTemplate restTemplate = new RestTemplate();
            String response = restTemplate.getForObject(url, String.class);
            parseResponse(response);
        }
    }

    // parse the response and store it in a variable
    private void parseResponse(String response) {
        //...       

        // get the block list of hostnames
        Map<String, List<String>> coloExceptionList = gson.fromJson(response.split("blocklist=")[1], Map.class);
        List<String> blockList = new ArrayList<String>();
        for(Map.Entry<String, List<String>> entry : coloExceptionList.entrySet()) {
            for(String hosts : entry.getValue()) {
                blockList.add(hosts);
            }
        }

        // store the block list of hostnames which I am not supposed to make a call
        // from my main application
        synchronized (workingHosts) {
            workingHosts.clear();
            workingHosts.addAll(availableHosts);
            workingHosts.removeAll(blockList);
        }
    }


    public DataResponse getDataResponse(URLGenerator gen, RestTemplate restTemplate, Class<?> returntype) {
        while (true) {
            boolean ok = false;
            String host = null;

            try {
                synchronized (workingHosts) {
                    if (workingHosts.isEmpty()) {
                        throw new IllegalStateException("There are no working hosts!");
                    }
                    host = workingHosts.removeFirst();
                    workingHosts.addLast(host); // put it at the end to create a round-robin load balancer.
                }

                String url = gen.generateForHost(host);

                DataResponse dr = restTemplate.getForObject(url, returntype);

                ok = true;

                return dr;
            } finally {
                if (host != null && !ok) {
                    synchronized(workingHosts) {
                        workingHosts.remove(host);
                    }
                }
            }
        }
    }

}

The point here is that all the logic is contained in one place....

You will need to add a way to schedule the background runnable every ten minutes, or when needed. That should be easy, with an ExecutorService....

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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