Issue 2521: Move DownloaderResponse access after succeeded() check #2549
Conversation
|
Looks good to me @abdulazizali77 (and not risky at all), @jaimecasero do you agree? |
| if(HttpStatus.SC_OK == sc){ | ||
| fsm.transition(message, ready); | ||
| } else if (HttpStatus.SC_NOT_FOUND == sc){ | ||
| fsm.transition(message, notFound); | ||
| } | ||
| } else { | ||
| if (logger.isDebugEnabled()) { | ||
| logger.debug("Rcml DownloaderResponse error : " + response.error() | ||
| + ", cause=" + response.cause().getMessage()); |
There was a problem hiding this comment.
im not sure "cuase()" will return an expcetion always in this branch, it may potentially throw NPE...
There was a problem hiding this comment.
@jaimecasero
Seems like theres only two places where DownloaderResponse is instantiated, so it doesnt look like anyone every passes a null to exception.
There was a problem hiding this comment.
im talkign abotu this expression "response.cause().getMessage()", and particularly how "response.cause()" may lead to null, not "response" object itself. I just dont want to chagne an NPE for other...
There was a problem hiding this comment.
What i pointed out was there does not seem to be any other Download failure instance where we instantiate the DownloaderResponse with null exception instead.
Line 187 is the only DownloaderResponse failure instance, so when it fails exception is always set. Hence there is no failure case where DownloaderResponse.cause() is null.
There was a problem hiding this comment.
Made an inline null check, dont really like that idiom for sure, but maybe easiest compromise. Advise if you think we should do away with it.
| } | ||
| //FIXME: what if these ifblocks arent in downloadingRcml? | ||
| if (response.succeeded()) { | ||
| int sc = response.get().getStatusCode(); |
There was a problem hiding this comment.
this should be safe now, as i suggested...
| if (downloadingRcml.equals(state) && fallbackUrl!=null) { | ||
| fsm.transition(message, downloadingFallbackRcml); | ||
| } else { | ||
| //TODO: should we rethrow response.error()? |
There was a problem hiding this comment.
exception handling is not always rethrowing... In this case we are both sending BYE potentially,and letting the FSM transition, so i would say no rethrow in this case...
There was a problem hiding this comment.
should we remove the TODO?
There was a problem hiding this comment.
i would certainly remvoe the TODO here
abdulazizali77
force-pushed
the
abdulazizali-BUG2521
branch
from
f76f9fb to
1d52e2e
Compare
@jaimecasero @FerUy Please have a look, apparently the mistake i made before was not moving the
get()into thesucceeded()block.Have added extra logging, advise if overkill.
There is already preexisting TechDebts for
UssdInterpreterso we will address further possible design issues there separately#2444
#2443