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

OkHttp implementations of ClientHttpRequestFactory / AsyncClientHttpRequestFactory [SPR-12893] #17492

Closed
spring-issuemaster opened this issue Apr 7, 2015 · 12 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented Apr 7, 2015

Luciano Leggieri opened SPR-12893 and commented

Alternative for existing implementation that uses the lightweight OkHttp library.


Affects: 4.1.6

Issue Links:

Referenced from: pull request #800, and commits 6468aa7, 69fc2a8

1 votes, 7 watchers

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 8, 2015

Rossen Stoyanchev commented

Pull request: #772

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 8, 2015

Rossen Stoyanchev commented

I know there is interest in in this from the side of the Spring Android project (see ANDROID-138). Roy Clarkson can you comment on the state of that, it looks like some code is already added? I guess we'd have to reconcile that and the pull request here. Either way it seems like something to consider adding to spring-web.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 8, 2015

Rossen Stoyanchev commented

We'll try and target 4.2.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented May 6, 2015

Juergen Hoeller commented

According to http://square.github.io/okhttp/ and its usage explanation, OkHttp seems to implement both HttpURLConnection and the Apache HttpClient APIs through adapter modules... So from that perspective, it can be used with our existing ClientHttpRequestFactory implementations already. I wonder what value a direct OkHttp based implementation would add? Increased configurability maybe?

Juergen

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented May 6, 2015

Luciano Leggieri commented

The HttpURLConnection wrapper (currently used for Android I think) is quite limiting. The Apache one has some drawbacks like:

  1. You need the HttpClient JARs which add more weight to a project (consider that OkHttp is lightweight).
  2. This one implements the asynchronous interface using the callbacks and future that OkHttp provides, but OKHttp-Apache only implements the synchronous HttpClient interface.
  3. OKHttp-Apache uses HttpClient 4.2.2, but the Spring interface requires 4.3 so not sure if it works out of the box.
@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented May 7, 2015

Rossen Stoyanchev commented

Maybe "try out OkHttp without rewriting your network code" was exactly what they meant, nothing more. It's a bit misleading having that on the home page with no further qualification but the README for the Apache adapter does say is quite limited .

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented May 7, 2015

Roy Clarkson commented

The implementation in Spring for Android uses the HttpURLConnection wrapper. I recommend against this, since it restricts the use of the OkHttp API features. For example, OkHttp offers its own interceptors which is a key feature missing in the android impl. Spring should expose a hook for these. I discussed with Arjen Poutsma this morning, and that sounds like the route he's taking.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented May 18, 2015

Juergen Hoeller commented

Thanks for the updated pull request, Arjen, just in time for 4.2! Assigning to Rossen for review...

Juergen

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented May 18, 2015

Arjen Poutsma commented

I've reworked Luciano Leggieri's PR into #800. I've merged the eight original classes into the three standard classes: request, response and factory, and also made some other minor changes.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented May 18, 2015

Luciano Leggieri commented

On the new PR, it seems that the request body is buffered, as it extends AbstractBufferingAsyncClientHttpRequest. Was there a logic for that?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented May 18, 2015

Arjen Poutsma commented

yes, there was: when the StreamingHttpOutputMessage was used, I ran into issues with the (Async)RestTemplateIntegrationTests that I could not solve. So I opted for this approach instead.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented May 18, 2015

Rossen Stoyanchev commented

Thanks Arjen Poutsma, I've merged the updated the PR into master.

Regarding body buffering, I see for Apache HttpComponents we have HttpComponentsAsyncClientHttpRequest (buffering) next to HttpComponentsStreamingClientHttpRequest. Would it make sense to have the same for OkHttp? It seems the one from the original PR was along the lines of HttpComponentsStreamingClientHttpRequest in terms of the upstream hieararchy.

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