-
Notifications
You must be signed in to change notification settings - Fork 130
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
base: master
Are you sure you want to change the base?
Core.async Support #113
Conversation
…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.
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 ( |
"Returns true if argument is a core.async channel" | ||
(partial satisfies? async-p/Channel)) | ||
|
||
(defmacro go? |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
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:
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.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.