Send request to network when cache entry parsing fails #321
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also invalidate the corrupted cache entry and set the request's entry to null.
jpd236
reviewed
Feb 25, 2020
| @@ -159,6 +159,15 @@ void processRequest(final Request<?> request) throws InterruptedException { | |||
| new NetworkResponse(entry.data, entry.responseHeaders)); | |||
| request.addMarker("cache-hit-parsed"); | |||
|
|
|||
| if (!response.isSuccess()) { | |||
There was a problem hiding this comment.
parseNetworkResponse is documented as being allowed to return null in case of an error, which means this would crash if that happens.
That being said, it seems likely that it would already crash in that situation:
- If entry.refreshNeeded(), then it will NPE on line 181 (response.intermediate=true).
- Otherwise, we call mDelivery.postResponse(). From there it depends on the ResponseDelivery implementation, but the default ExecutorDelivery seems like it would crash checking mResponse.isSuccess().
So maybe it's worth filing a separate bug to investigate whether we handle that scenario correctly or should just update the Javadoc to require that callers use Response.error rather than returning null?
|
Given that returning null crashes already, I think it's unlikely that anyone uses a Request that returns null on error. Then it seems better to update the javadoc to reflect the fact that null is already not really supported. But I'll defer to your judgement. |
|
Filed #322 for the Javadoc. |
This was referenced Mar 9, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Also invalidate the corrupted cache entry and set the request's entry to null.
This is probably quite rare, but we should still rather send the request to the network than keep trying to return a potentially corrupted cache entry until it expires.