0
\$\begingroup\$

I am trying to get an Auth ticket from a 3rd party server. I am using FutureTask to perform the task. However, if I don't get response from the server after 10 seconds, I want to timeout the call. At the same time, I want to kill the Thread which was busy in fetching the Auth Ticket from 3rd party, but got stuck because of some issue. Here is the simplified code.

public String getAuthTicket(final String userName) throws Exception{
    String authTicket=null;
    final FutureTask<String> future =
            new FutureTask<String>(new Callable<String>() {
                public String call() throws Exception {                             
                    log.info("calling server to get trusted ticket.");                                              
                    if(Thread.interrupted()){
                        log.info("Call to server got interrupted.");
                        return "-1";
                    }
                    String authTicket = getTrustedTicket(userName);
                    return authTicket;
                }
            });

    final Thread thread = new Thread(future);
    thread.start(); 
    String authTicket ="-1";    

    try {
        authTicket = future.get(10000, TimeUnit.MILLISECONDS);
        log.info("authTicket: "+ authTicket);
        return authTicket;
    } catch (Exception e){
        log.warning("Timeout occured while waiting to get auth ticket: Reason:"+ e);
        future.cancel(true);            
    }

    return authTicket;
}

Could anyone please review my code?

\$\endgroup\$
2
\$\begingroup\$

Instead of starting a new thread you can instead create a executor which will take Runnables and reuse threads as needed. you can create a executor usign one of the Executors' static factories.

This code relies on getTrustedTicket() responding correctly to a thread interruption. Which you didn't include so I can't judge it. (A lot of IO functions don't respond to it by default).

The catch-all when waiting on the result is not a great thing. You should instead catch only the specific exceptions you can get and only declare those you throw.

try {
    authTicket = future.get(10000, TimeUnit.MILLISECONDS);
    log.info("authTicket: "+ authTicket);
    return authTicket;
} catch (InterruptedException e){
    log.warning("Current thread interrupted while waiting to get auth ticket: Reason:"+ e);
    future.cancel(true);
    Thread.currentThread().interrupt(); //reset interrupted state or rethrow the exception.
} catch (ExecutionException e){
    log.warning("Exception occured getting the auth ticket: Reason:"+ e.getCause());
} catch (TimeoutException e){
    log.warning("Timeout occurred while waiting to get auth ticket: Reason:"+ e);
    future.cancel(true);
} 
\$\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.