retrofit2 extra > NullPointerException on handling "204 No-Content" http response

[anton-fedorov]

Hello,

It seems that AsyncHttpClientCall.java:255 creates response body to map to OkHttp only if there was actually response in AsyncHttp call.

This unfortunately leads to NullPointerException if you try to parse 204 HTTP Response (No Content).

This happens due to the fact that Retrofit's OkHttpCall.java#L202 tries access rawBody w/o any null checks.

Would it make sense to have okHttpBody = ResponseBody.create(contentType, new byte[0]) in case if response was 204/205?

[slandelle]

Probably. Contributions welcome :)

[danesavot]

I am experiencing the same issue. Any workaround for a quick fix?

[slandelle]

What about contributing like I suggested? This module is purely a contribution and I don't use Retrofit myself.

[danesavot]

I will try to raise the PR.

[slandelle]

I'm not familiar with retrofit and okhttp, but it really looks a retrofit bug to me.
I don't get why we should set a okhttp3.ResponseBody when response doesn't have a body, all the more as the ContentType constructor parameter is required, and we obviously don't have one here.

Please report to the retrofit project.

[zeldigas]

@slandelle just a quick question - would you accept PR for async-http-client module or the only acceptable way for you would be fix of retrofit itself?

[slandelle]

@zeldigas I'd like to have retrofit authors' opinion about this issue. Has anyone reached them and/or opened an issue there (I won't)?

[zeldigas]

got it. let me check their javadocs in more details first. I agree that it's counter intuitive to have a body object when there is no body at all, but probably it's by design and we should adjust AHC adapter for their lib...

[slandelle]

I'm super against pushing design mistakes and bugs on consumers instead of fixing them upstream.

But then, I'm not familiar with Retrofit and there's maybe a good reason for the way it currently works.
Or, it might indeed be something Retrofit authors would agree to fix on their side.
Or if things work as intended, maybe they could explain the proper way to do this.

[zeldigas]

Heres docs for okhttp3.Response#body() :

Returns a non-null value if this response was passed to Callback.onResponse or returned from Call.execute(). Response bodies must be closed and may be consumed only once.
This always returns null on responses returned from cacheResponse, networkResponse, and priorResponse().

And in our problem place we are constructing response exactly for callback, here is code excerpt from retrofit

@Override public void onResponse(okhttp3.Call call, okhttp3.Response rawResponse) {
        Response<T> response;
        try {
          response = parseResponse(rawResponse);
....
Response<T> parseResponse(okhttp3.Response rawResponse) throws IOException {
    ResponseBody rawBody = rawResponse.body();

    // Remove the body's source (the only stateful object) so we can pass the response along.
    rawResponse = rawResponse.newBuilder()
        .body(new NoContentResponseBody(rawBody.contentType(), rawBody.contentLength())) 
//                                       ^--- NPE is here

So I'd say API is strange a bit, especially taking into account that body is marked Nullable, but javadocs explicitly says that body should not be null for responses passed to callbacks, so I'd simply fix it in AHC extra module to be more strictly compliant with retrofit/okhttp contract.
@slandelle your thoughts on this?

[slandelle]

¯_(ツ)_/¯

Also, it a pity NoContentResponseBody is package private.
PR welcome

[zeldigas]

Deal, I'll send PR this week.