Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Empty body on HTTP error response #260

Closed
stromnet opened this issue Aug 31, 2015 · 22 comments · Fixed by #1182
Closed

Empty body on HTTP error response #260

stromnet opened this issue Aug 31, 2015 · 22 comments · Fixed by #1182
Labels
bug Unexpected or incorrect behavior help wanted Issues we need help with tackling

Comments

@stromnet
Copy link
Contributor

I had a problem where the response body would come up empty, when the server responded with 401:

2015-08-31 17:11:01.168 [MyInterface#myMethod] <--- HTTP/1.1 401 Unauthorized (12ms)
2015-08-31 17:11:01.169 [MyInterface#myMethod] Date: Mon, 31 Aug 2015 15:11:01 GMT
2015-08-31 17:11:01.169  [MyInterface#myMethod] Content-Length: 71
2015-08-31 17:11:01.170  [MyInterface#myMethod] Content-Type: application/json
2015-08-31 17:11:01.170  [MyInterface#myMethod] <--- END HTTP (0-byte body)

As indicated by content-length header, the body should be 71 bytes, and it is (verified with tcpdump).
The empty body caused a NPE when the ErrorDecoder had nothing to decode and returned null, which SynchronousMethodHandler then tried to throw (line 121).

During previous testing, with 200 OK it worked fine, if I recall correct.

This was with the Default client. Using OkHttpClient instead, the issue disappeared.

@codefromthecrypt
Copy link
Contributor

codefromthecrypt commented Sep 3, 2015 via email

@stromnet
Copy link
Contributor Author

stromnet commented Sep 4, 2015

Actually, no. I tried the following in BaseApiTest:

  @Test
  public void handlesErrorBody() throws InterruptedException {
    server.enqueue(new MockResponse().setResponseCode(401).setBody("foo"));

    String baseUrl = server.getUrl("/default").toString();

     try {
        Feign.builder()
            .logLevel(Logger.Level.FULL)
            .client(new Client.Default(null, null))
             .errorDecoder(new ErrorDecoder.Default())
             .target(MyApi.class, baseUrl).get("foo");
        fail();
     }catch(FeignException ex) {
         assertThat(ex.getMessage())
             .isEqualTo("status 401 reading MyApi#get(String); content:\nfoo");
     }
  }

And this passes fine, the response gets through without a problem.. Weird! When I had the issue, the remote end was a Jersey-based server. As I mentioned, the tcpdump on localhost showed the actual bytes being sent.. The only difference was that I was using a JacksonDecoder, but I tried that as well and couldn't reproduce the error.
Even tried targeting the test towards the Jersey server, and it still worked. Could this have something to do with the runtime environment in which the test/HttpURLConnection is running in? Not sure if there are any customizations in the faulty environment, it's a spring boot environment so it might set all sorts of weird things (if even possible).

Sidenote: even though I specify Full logging level (and forcing the Logger-level on feign.Logger to All), I don't see the http dump as I did in my original code (which uses the Slf4jLogger).. adding a System.out.println line in the log method did show proper output though.

@stromnet
Copy link
Contributor Author

stromnet commented Sep 4, 2015

Tested with both v8.7.1 and master.

@codefromthecrypt
Copy link
Contributor

I wonder if something is overriding the URLStreamHandlerFactory like how we do in okhttp:

URL.setURLStreamHandlerFactory(new OkUrlFactory(okHttpClient));

This would manifest as a different type of object (like in a different package) for HttpURLConnection.

@emilnkrastev
Copy link

This is not a problem in feign - this is how HttpURLConnection works. Currently in the feign.Default client there is a streaming mode enabled. You can see in the sun.net.www.protocol.http.HttpURLConnection following lines of code :
if (respCode == HTTP_UNAUTHORIZED) { if (streaming()) { disconnectInternal(); throw new HttpRetryException (RETRY_MSG2, HTTP_UNAUTHORIZED); }

So if the streaming is enabled and you have 401 HTTP response code you will get empty errorStream, because there is no initialization. The feign client will try to get the errorStream as a body because there is a check
if (status >= 400) { stream = connection.getErrorStream(); } else { stream = connection.getInputStream(); }

Currently I removed the streaming in the cient.
connection.setFixedLengthStreamingMode(contentLength); connection.setChunkedStreamingMode(8196);

@vincent-ren

This comment has been minimized.

@frankskywalker

This comment has been minimized.

@vincent-ren

This comment has been minimized.

@kdavisk6
Copy link
Member

This feels like a bug to me. We may need more null checking before blindly reading the error stream.

@kdavisk6 kdavisk6 added bug Unexpected or incorrect behavior help wanted Issues we need help with tackling labels Sep 14, 2018
@ColtonPaysafe
Copy link

ColtonPaysafe commented Nov 5, 2018

This is caused by https://github.com/OpenFeign/feign/blame/master/core/src/main/java/feign/Client.java#L85... Setting allowUserInteraction to false immediately closes the stream and causes getErrorStream to return a null.

See also: https://stackoverflow.com/a/44423631/8651856

@ColtonPaysafe
Copy link

My recommendation would be to release a minor version with Request$Options containing a allowUserInteraction boolean (default to false). In the next major release, change the default to true.

@344310362
Copy link

这个修复啥时候发布,怎么就把参数定死了。

@rage-shadowman
Copy link
Contributor

rage-shadowman commented Dec 6, 2018

@344310362 english comments are more likely to be understood by most of the people following this project.

Google translate isn't very good here as it turns that into:

When this fix is released, how can I set the parameters to death?

Perhaps a better translation of "death" would be "null"?

@kdavisk6
Copy link
Member

There issue is marked as help-wanted, indicating that if someone wants to take a look and try to fix this, please do and raise a PR.

@stromnet
Copy link
Contributor Author

From my side I won't do much, not using it anymore. Not even sure where I used it.. :)

@kdavisk6
Copy link
Member

After looking into this recently, I can see that this behavior is specific to JDK versions prior to 1.8. I'm going to close this issue and recommend that user's test with JDK 1.8.

@sajjaadalipour
Copy link

Hey guys. I had this problem and set the OkHttp to Openfeign and fixed.

In order to know how can use from OkHttp in Openfeign refer to here

@verils
Copy link

verils commented Feb 28, 2020

Unfortunately this 'feature' has remained to JDK 11. I just tested

verils added a commit to verils/feign that referenced this issue Mar 3, 2020
* Add a property `allowStramingMode` in `Request.Option`.

* Optional to disable the streaming mode for `HttpURLConnection`.

* The property is default to `true` for compatibility.

Fix OpenFeign#260
verils added a commit to verils/feign that referenced this issue Mar 3, 2020
* Add a property `allowStramingMode` in `Request.Option`.

* Optional to disable the streaming mode for `HttpURLConnection`.

* The property is default to `true` for compatibility.

Fix OpenFeign#260
velo added a commit that referenced this issue Mar 5, 2020
* Configurable to disable streaming mode for Default client

* Add a property `allowStramingMode` in `Request.Option`.

* Optional to disable the streaming mode for `HttpURLConnection`.

* The property is default to `true` for compatibility.

Fix #260

* Fix a merging problem

* Make the new option `disableRequestBuffering` to Default client

* Move it from `Request.Options` to `Default` client
@nada-gamal
Copy link

Did anybody had a workaround to resolve this issue? It is existing I am using JDK 11 and Openfeign 2.2.6. I have tried with other clients other than the default Okhttp and Apache HttpClient and still getting empty response body with 401 error response.

@minhnguyenvan95
Copy link

Did anybody had a workaround to resolve this issue? It is existing I am using JDK 11 and Openfeign 2.2.6. I have tried with other clients other than the default Okhttp and Apache HttpClient and still getting empty response body with 401 error response.

Same too me , response code = 401 the response body return null, JDK 17, Feign 3.1.4

@velo
Copy link
Member

velo commented Jul 6, 2023

Did anybody had a workaround to resolve this issue? It is existing I am using JDK 11 and Openfeign 2.2.6. I have tried with other clients other than the default Okhttp and Apache HttpClient and still getting empty response body with 401 error response.

Same too me , response code = 401 the response body return null, JDK 17, Feign 3.1.4

Feign 3? That is 10 releases old. Is like trying to run spring 1 with jdk 17.

@andreaippo
Copy link

I can confirm that using the okhttp client the problem disappears (https://github.com/OpenFeign/feign/blob/6408e3ef9664ee6594c12d9e3c2cc91fd29119d0/okhttp/src/main/java/feign/okhttp/OkHttpClient.java)

To add it to our project we had to:

add the pom dependency:

<dependency>
<groupId>io.github.openfeign</groupId>
<artifactId>feign-okhttp</artifactId>
</dependency>

And in the feign config bean, make sure to init your Feign with sth like:

Feign.builder().client(new feign.okhttp.OkHttpClient())...;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unexpected or incorrect behavior help wanted Issues we need help with tackling
Projects
None yet
Development

Successfully merging a pull request may close this issue.