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

Core.async Support #113

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

dthume
Copy link

@dthume dthume commented Mar 23, 2014

As per #91 I've added core.async support to liberator. Simply return true from the new :async? decision, then return a core.async channel from any subsequent resource function in order to enable non-blocking mode. I'll raise a separate pull request for accompanying docs.

Please note the following:

  1. The :async? decision result is not actually used internally by liberator at the moment, but I've kept it as part of the API to allow us to potentially experiment with different approaches later.

  2. I've tried a number of different approaches to implementing this - this pull request contains the least disruptive (to the code base) of those approaches - the new support is backwards compatible with existing liberator users, and core.async support is entirely optional (albeit it is required on the classpath), at the price of one somewhat ugly macro. Internally the majority of the flow is unchanged, and although some of the larger functions have been split up in order to keep "pure" logic out of the go blocks, the implementation of those functions has not changed.

Other approaches that I tried yielded somewhat better performance for traditional (sync) requests (almost as fast as liberator master is at the moment), but the resulting code complexity / ugliness wasn't worth it IMO, and the performance of the current code is acceptable (again IMO) given that it is, in general, completely dominated by user code. That said I'm quite happy to use a more invasive approach if folks feel strongly about it. The perforate tests can be used to compare the two styles, and branches, assuming PR #112 is accepted.

dthume added 2 commits March 16, 2014 10:12
…a channel, in which case resource execution

will return a default response containing a :body whose value is a channel onto which the eventual response will
be placed.
@dthume
Copy link
Author

dthume commented Mar 25, 2014

I've taken a stab at updating the trace support, and it seems to be somewhat working again, although there are issues with some of the generated ids (svg.getElementById("post") whereas the element name in the svg seems to be trace_node5, but to be honest it may just be some misunderstanding on my part; I'll keep looking...

"Returns true if argument is a core.async channel"
(partial satisfies? async-p/Channel))

(defmacro go?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this macro end with a "?". It doesn't seem like a predicate.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I borrowed the pattern from a blog by David Nolen. The ? for go? and <? is simply to indicate that they might fail; the former catching any exceptions and returning them as the result, and the latter rethrowing any exceptions which are pulled from the channel.

@ordnungswidrig
Copy link
Member

I'll close this PR. All the work done here is great, but at the moment out of scope. In the future there might be a more flexible execution model for liberator which will also support async like in http-kit, aleph or based on async.

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

Successfully merging this pull request may close these issues.

3 participants