HiveBrain v1.2.0
Get Started
← Back to all entries
debugjavaMinor

Varnish - HttpClient Exception Handling

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
httpclienthandlingvarnishexception

Problem

I'm consuming some messages from a rabbit queue. Within the consumer/processor I need to invalidate a varnish cache and requeue the message if the purge failed.
I'm using a local purge proxy.
The consumer is a Dropwizard microservice but I'm using a BasicHttpClient because JerseyClient doesn't allow me (AFAIK) to set PURGE as http verb.

I'm trying to improve my error handling.. Is throwing an IllegalStateException fine or should I improve it?
Thanks

public void invalidateVarnishCache( String level, String idUrl ) throws IOException {
    try{
        String xPurgeRegex = level+"/"+idUrl+"$";
        Header header = new BasicHeader( "X-Purge-Regex", xPurgeRegex );
        BasicHttpRequest purgeRequest = new BasicHttpRequest("PURGE", "/" );
        purgeRequest.setHeader(header);

        LOGGER.log( LogEntries.VarnPurge, level, idUrl, host, xPurgeRegex ); 

        response = httpClient.execute(host, purgeRequest);

        entity = response.getEntity();

        int statusCode = response.getStatusLine().getStatusCode();

        if( !is2XXResponse( statusCode ) ){

            int respLength = entity.getContent().available();
            byte[] errorResp = new byte[ respLength ];
            entity.getContent().read(errorResp, 0, respLength);

            LOGGER.log( LogEntries.VarnPurgeErrResp, statusCode, new String(errorResp) ); 

            throw new IllegalStateException("Varnish local proxy returned response with status code "+statusCode+" from Varnish backend. Consumed message will be requeued.");
                      /////////////////////
        }else{
            LOGGER.log( LogEntries.VarnPurgeSuccessResp, statusCode ); 
        }

        EntityUtils.consume(entity);

    }catch( IOException ex ){
        LOGGER.log( LogEntries.VarnPurgeException, ex ); 
        throw ex;
    }
}


This is the consumer..

```
public MessageResult apply(UpdateMessage t) {

if( !agentValidator.validMessage( t ) ){
return MessageResult.ACK; // discar

Solution

The Consumer

The nested try-catch blocks look weird and useless. Since purgeVarnishBeforePcsRequest and getLatestStateOfResource throw exceptions, they may be caught in the outer catch block.

try {
  pcsConnector.purgeVarnishBeforePcsRequest( level, idUrl );
  response = connector.getLatestStateOfResource( urlRequest );
  // .....
  return MessageResult.ACK;
} catch (Exception e) {
  // logger
  return MessageResult.NACK_REQUEUE;
}


By the way, catching of Exception type might be a bad coding practice, but I'd not discuss it here because the context is not enough to judge.

invalidateVarnishCache

The only purpose of the try-catch block in the original code is to trace the IOException using a hypothetical commented logger and then re-throw it. I doubt that the block itself is useful, because exactly the same exception is re-thrown and must be handled somewhere among the callers and even probably also logged by them. Wouldn't it be redundant?

Concerning the IllegalStateException, the problem is that it is a RuntimeException and the caller can miss to handle it properly, with hardly predictable consequences.

I don't know what is the range of response statuses that this sort of proxy can return, but your implementation does not make distinction between 4xx and 5xx codes, which may lead either to a client-side error (your request is invalid, 4xx) or to a server-side error (5xx), which is semantically close to an I/O exception. For the latter, I can suggest to wrap the response message into a IOexception and re-throw it. For the former, it's still up to you to decide how to handle it, probably just logging is enough.

Code Snippets

try {
  pcsConnector.purgeVarnishBeforePcsRequest( level, idUrl );
  response = connector.getLatestStateOfResource( urlRequest );
  // .....
  return MessageResult.ACK;
} catch (Exception e) {
  // logger
  return MessageResult.NACK_REQUEUE;
}

Context

StackExchange Code Review Q#126769, answer score: 2

Revisions (0)

No revisions yet.