Retry after exception - SCA compliant

[Rajdeep Biswas]

Here's the logic I used:

int retries = config.get("retries");
Response resp = null
do {
    try {
        resp = operation.execute();
        retries = 0;
    } catch (Exception ex) {
        if (ex instanceof ExceptionA) {
            retries--;
            LOGGER.info("Integrity exception, can be retried");
            if (retries == 0) {
                LOGGER.info("Retry exhausted");
                throw ex;
            }
            LOGGER.info("Retrying operation, retry count " + ledgerRetry);
        } else {
            throw ex;
        }
    }
} while (retries > 0);
return resp;

Number of retries is considering original operation as well. But the problem is that

if I return from try block directly without assigning anything, then SCA (Fortify for me) reports that the variable retries is not read (in success flow), and
if I assign and do as above, then SCA shouts about the immediate reassignment of value to the retries variable without even reading it.
How do I write it nicely?

[Campbell Ritchie]

Rajdeep Biswas wrote:. . .        if (ex instanceof ExceptionA) . . . . . .

Did you really use instanceof there? That isn't right, surely? You catch a particular exception type. I am surprised the code verifying tool complained about retries and didn't say anything about instanceof. The bit about retries being 0 is also difficult to read (line 11).
I presume you are sure that particular kind of Exception allows you to retry immediately without altering anything else?

[Rajdeep Biswas]

The instanceof is not there, just I added to make others understand. There is a method call that checks the contents of the exception.

[Campbell Ritchie]

Please post the actual code you are running. We can hardly help on different code.

[Rajdeep Biswas]

As I said, it calls a boolean returning method passing on the exception object, and in the method we check for exception message contents and other things. But I wonder why should it matter because the assumption is that if the condition is met, i.e. if the method returns true, then we think of retry, anything else we throw the exception. My concerns I have put.

int retries = config.get("retries");
Response resp = null
do {
    try {
        resp = operation.execute();
        retries = 0;
    } catch (Exception ex) {
        if exceptionCriteriaMet(exception) { //a method which checks for some details in the exception, like the wrapped exception inside a generic application exception and the message details etc.
            retries--;
            LOGGER.info("Integrity exception, can be retried");
            if (retries == 0) {
                LOGGER.info("Retry exhausted");
                throw ex;
            }
            LOGGER.info("Retrying operation, retry count " + ledgerRetry);
        } else {
            throw ex;
        }
    }
} while (retries > 0);
return resp;

[Rajdeep Biswas]

Considerations:

The first call should be independent of whatever value we read for 'retries'

Duplicate code should be avoided, and avoiding recursion will be nice too.

May be a simple thing, but I am not catching it probably. Please suggest.

[Dave Tolls]

To be honest, I think that code looks fine to me.
This looks more like Fortify having issues identifying the flow.
It essentially doesn't understand that retries is for handling the failure flow.

[Liutauras Vilda]

Dave Tolls wrote:To be honest, I think that code looks fine to me.

Apart maybe from the line 8, which supposed to prevent code from compiling.

[Dave Tolls]

This bit?

if exceptionCriteriaMet(exception)

It's clunky, but if you've already wrapped the actual exception types that you can recover from, then it's fair enough I suppose.

Didn't think it was preventing anything from compiling, though.

[Campbell Ritchie]

Agree; that is a great improvement on the previous code. It looks strange to have a method for checking an Exception, however. You have some repeated code, logging the exception thrice and rethrowing it twice. You can probably simplify that to LOGGER.info(exceptionMessage); or similar.
If you are simply testing the type of the exception, what about

int retries = config.get("retries");
Response resp = null
do {
    try {
        resp = operation.execute();
        retries = 0;
    } catch (ExceptionA ex) {
            retries--;
            LOGGER.info("Integrity exception, can be retried");
            String message = (retries == 0) ? "Retry exhausted" : "Integrity exception, can be retried");
            LOGGER.info(message);
            if (retries == 0) {
                throw ex;
            }
            LOGGER.info("Retrying operation, retry count " + ledgerRetry);
        } catch (Exception ex) {
            throw ex;
        }
    }
} while (retries > 0);
return resp;

Please check very carefully because I might have got the flow of control completely wrong. I also suspect I have got one { too many. Why do I have the catch in lines 16‑18? I suspect I can miss that out and simply allow the exception to propagate.

[Rajdeep Biswas]

That is not my problem at all. Please read the last statements in original post.

[Liutauras Vilda]

Dave Tolls wrote:This bit?

Yes. I was just saying, if OP claims it is a real code he's using currently, then it supposed to fail to compile as there are missing parentheses for "if" statement. But I might misunderstanding something what is discussed here as didn't read thread carefully.

[Dave Tolls]

Liutauras Vilda wrote:
Yes.

Ohhhhh!
I missed that.


Actually...would you have a different result with a while loop?
Or is it because it's Friday and I'm not thinking straight?

[Dave Tolls]

No...I'm right I think.

At the moment if 'retries' is 2, then it goes around a maximum of 2 times (so it's actually only retrying once).
If it were a while loop you would get the same result...2 attempts.

[Rajdeep Biswas]

Ok, I am going to put things once again.

1. I am reading number of retries from outside.
2. Now, dont take it literally. If I read a value of 3 in retries, then, sat if we have the known exception in each execution, then total number of times the operation is executed is 3 (including original request, effectively meaning 2 retries instead of 3). Please ignore this thing. I will modify it later.
3. The way catch is defined and handled also is not in the scope of my discussion.

Here I am reiterating my issue:

1. If I do like this:

//....
int retries = config.get("retries");
do {
    try {
        return operation.execute(); //problem *****
    } catch (Exception ex) {
    //...
    }
} while (retries > 0);
return null;

then I am getting issue that variable "retries" is not read. So to resolve this, I tried below:
2.

//....
int retries = config.get("retries");
Response res = null;
do {
    try {
       res = operation.execute();  
       retries = 0; //problem *****
    } catch (Exception ex) {
    //...
    }
} while (retries > 0);
return res;

this now removes above problem but introduces another problem - redundant initialization of "retries", i.e. without being used, retries is being reassigned another value.
So I am looking for a clean approach to write it in a better way.
Here are my considerations:

The first call should be independent of whatever value we read for 'retries'

Duplicate code should be avoided, and avoiding recursion will be nice too.

[Dave Tolls]

Rajdeep Biswas wrote:Ok, I am going to put things once again.

I understand what your issue is.
I think you might be putting too much faith in the code analysis tool...though having said that, changing your loop to a while rather than a do-while will fix all those issues that Fortify is complaining about.  The only situation it will cause a problem is if retires is set to 0 in the config.  Since that may well be an issue (I wouldn't like to trust people not to change things) then either do a check on 'retries' or stick with your current code and ignore Fortify.

[Rajdeep Biswas]

Alright Dave. Checking and setting retries to 1 for wrong values was in my mind. Just thought if we have a better piece of code in the same manner as do ... (once), and while ...(retries) - try-again. It wont be an issue though, I will go with former only if thats that.