clj-http icon indicating copy to clipboard operation
clj-http copied to clipboard

Async requests do not return a response map

Open dAnjou opened this issue 5 years ago • 3 comments

Hi,

when doing an async request like:

(let [response (defer (http/get "http://example.com" {:async? true} (fn [_] _) (fn [_] _))])

response is org.apache.http.HttpResponse and not a Ring response map.

In our use case we're using the code above in our tests.

Can the return value of an async request also be wrapped in such a map?

dAnjou avatar Aug 08 '19 17:08 dAnjou

Unfortunately there isn't a way to do out of the box because of the async nature. It would break the functional abstraction of a response/response map. The map would not have headers or body available.

However, you can achieve a similiar functionality in tests by putting the async response inside a promise and then blocking in your tests until the result comes back with deref.

In terms of code, it'd be something like this:

(let [promise-response (promise)]
  (http/get "..." {:async? true} (fn [response] (deliver promise-response response))]
  (deref promise-response 1000)  ; wait up to 1000ms for the response
  )

rymndhng avatar Aug 09 '19 19:08 rymndhng

Hi Raymond, thanks for responding!

The code I've shown is not the real code. In our case the async request is done inside a function which we call in our test where we can't inject a custom callback.

However, our function returns whatever http/get returns which happens to be a Future<HttpResponse>. So, we can and do defer that but then it's not a Ring response map.

Wouldn't it be possible for http/get to return the kind of promise that you're describing?

dAnjou avatar Aug 10 '19 18:08 dAnjou

That does seem like a reasonable request.

I can think of two ways this could be done:

Option 1: Introduce no new options and replace the current BasicFuture response object.

I'm uneasy about this breaking change since the current returned object is a org.apache.http.message.BasicHttpResponse. This original API was designed to mimic async-ring ring, we should respect this.

Option 2: Introduce a different request option to return the response as a fully defined future.

We could create a new option called :async-future which uses uses the existing async machinery that wraps the existing :async API as a tidy future. This approach avoids breaking the existing API contract, allowing users to upgrade at their leisure.


I am leaning towards Option 2 to avoid breaking changes. Do you have any thoughts on this? cc-ing: @dakrone

rymndhng avatar Aug 10 '19 23:08 rymndhng