Skip to content

Async requests do not return a response map #512

New issue

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

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

Already on GitHub? # to your account

Open
dAnjou opened this issue Aug 8, 2019 · 3 comments
Open

Async requests do not return a response map #512

dAnjou opened this issue Aug 8, 2019 · 3 comments

Comments

@dAnjou
Copy link

dAnjou commented Aug 8, 2019

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?

@rymndhng
Copy link
Collaborator

rymndhng commented Aug 9, 2019

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
  )

@dAnjou
Copy link
Author

dAnjou commented Aug 10, 2019

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?

@rymndhng
Copy link
Collaborator

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

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

2 participants