Making static method Synchronized or Not

Question

I have a webservice call to get an authorization token and use it for subsequent webservice calls. Now what we had done earlier was whenever we make any web service call, we first make the token web service and then make the call for actual web service.

Method to get the token is as shown below. Basically what this code does is call the webservice to get the token and using GSON parse the response and get the token.

public static String getAuthTicket() {
  String authTicket = null;
  HttpResponse httpResponse = getAuthResponse();
  String body;
  if (httpResponse.getStatusLine().getStatusCode() == 200) {
    try {
      body = IOUtils.toString(httpResponse.getEntity().getContent());
      Gson gson = new GsonBuilder().disableHtmlEscaping().create();
      ResponseTicket responseTicket = gson.fromJson(body, ResponseTicket.class);
      authTicket = responseTicket.getTicket();
    } catch (UnsupportedOperationException e) {
      LOGGER.error("UnsupportedOperationException : ",e);
    } catch (IOException e) {
      LOGGER.error("IO Exception : ",e);
    }
  }
  return authTicket;
}

This has obviously led to performance issue. Hence the party who is providing the webservice to get the token has made the token valid for 30 minutes.

So in the above method what we are thinking is to put the token in cache along with the time and check if the current time - cache time is less than 30. If time is greater than 30 we will make service call to get token and update the token with timestamp in cache.

The only thing is I am fearing is about synchronization, so that I dont get corrupt authtoken due to race condition.

I am thinking to make this static method as synchronized. Do you think is there any other better way.

Answer 1

The answer is: it depends.

Race conditions occur when more than one thread is accessing shared data at the same point in time. So, when you would have code such as:

private final Map<X, Y> sharedCache = new HashMap<>();

public static getAuthTicket() {
  if (! sharedCache.containsKey...) {
    sharedCache.put(...
...

You would be subject to a race conditions - two threads could come in at the same time, and update that shared map at the very same time; leading to all kinds of problems.

When I get your code right - you would have something similar:

private static String cachedToken = null;

public static getAuthTicket()  {
  if (cachedToken == null || isTooOld(cachedToken)) {
    cachedToken = getAuthTicketForReal();
  }
  return cachedToken;
}

You probably do not want that two threads call getAuthTicketForReal() in parallel.

So, yes, making that method synchronized is a valid approach.

Where: the real question is: is it sufficient to add that keyword? Given my code - the answer is yes. You simply want to avoid that this cache is setup "in parallel" by more than one thread.

Finally: in case you are worried about the performance impact of using synchronized here - simply forget about that. You are talking about a multi-second "network based" operation; so you absolutely do not worry about the milli second of overhead that synchronized might have (making up this number - the key thing: it is so small that it doesn't matter in the context of the operation you are doing).

Regarding your comment: of course, using synchronized means that the JVM will serialize calls to that method. This means when this method needs 1 minute to return - any other calls to that method will block for that 1 minute.

In that sense; it might be a good exercise to look into ways of writing up this method in a way that does not require synchronized on method level. For example by using data structures that can deal with multiple threads manipulating them.