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

HttpComponentsStreamingClientHttpRequest fails if restTemplate has interceptors [SPR-17113] #21650

Closed
spring-issuemaster opened this issue Aug 1, 2018 · 5 comments

Comments

@spring-issuemaster
Copy link
Collaborator

@spring-issuemaster spring-issuemaster commented Aug 1, 2018

raoofm opened SPR-17113 and commented

need to set Interceptors as empty, since InterceptingHttpAccessor.getRequestFactory returns InterceptingClientHttpRequestFactory which in turn makes an instance of InterceptingClientHttpRequest which does not extend StreamingHttpOutputMessage and AbstractHttpMessageConverter.write checks if (outputMessage instanceof StreamingHttpOutputMessage) { which will be false and ResourceHttpMessageConverter.writeInternal which calls outputMessage.getBody() where outputMessage is an instance of HTTPOutputMessage and not StreamingHttpOutputMessage, should return something like InterceptingStreamingClientHttpRequestFactory For now when we set empty Interceptors, returns the correct delegate (HttpComponentsClientHttpRequestFactory)


Issue Links:

  • #20926 InterceptingClientHttpRequest is always buffered despite the delegate
  • #17312 All HttpMessageConverters should support StreamingHttpOutputMessage
@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Aug 1, 2018

raoofm commented

May be related to

#312

#15356

#17312

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Aug 1, 2018

raoofm commented

InterceptingClientHttpRequest.java
public ClientHttpResponse execute(HttpRequest request, byte[] body) throws IOException {
ClientHttpRequest delegate = requestFactory.createRequest(request.getURI(), request.getMethod());
delegate is org.springframework.http.client.HttpComponentsStreamingClientHttpRequest
StreamUtils.copy(body, delegate.getBody());
throws
throw new UnsupportedOperationException("getBody not supported");
@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Aug 2, 2018

Rossen Stoyanchev commented

I don't think they're meant to work together. A ClientHttpRequestInterceptor expects the full body aggregated as a byte[] which defeats the purpose of StreamingHttpOutputMessage to stream the body.

The WebClient is the recommended solution for proper streaming support.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Aug 2, 2018

raoofm commented

I think there is a big gap here and I would like to have a discussion before this issue is resolved.

Do you think that if a client chooses an option of streaming then it should give up interceptors?

Using interceptors is a common scenario, people use it for logging, metrics, authentication, customHeaders etc

We use feign and it has a metrics interceptor, even if I remove feign I would still need an interceptor for adding auth/customHeaders.

Ideally I can think of a few ways to handle this

  • A StreamingClientHttpRequestInterceptor which doesn't accept a byte[] body
  • Existing interceptor ignoring body for streaming clients
  • Simply the getBodyInternal() in HttpComponentsStreamingClientHttpRequest should be NoOp rather than throwing UnsupportedOperationException

Open to better solutions.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

@spring-issuemaster spring-issuemaster commented Aug 2, 2018

Rossen Stoyanchev commented

I agree with the gap but the RestTemplate is not the answer. If you need streaming, use the WebClient. Unfortunately there are no easy answers. A streaming interceptor implies creating a whole new contract and a parallel mechanism unrelated to the current interceptor contract which requires the body to be buffered. Even then there are more issues with streaming (e.g. #17232) and at best this whole new mechanism would work for the Apache client only. This isn't a route we're going to pursue with the WebClient available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.