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

missing tests for parallel caching feature #100

Open
jjb opened this issue Oct 7, 2014 · 3 comments
Open

missing tests for parallel caching feature #100

jjb opened this issue Oct 7, 2014 · 3 comments

Comments

@jjb
Copy link
Contributor

jjb commented Oct 7, 2014

Looks like in a83b39c, the if env[:parallel_manager] condition and the cache_on_complete method it uses were not tested.

The feature itself seems like a bit of a smell — why would the concurrent case not work with threadsafe code?

@jjb jjb mentioned this issue Oct 7, 2014
@mislav
Copy link
Contributor

mislav commented Oct 7, 2014

The parallel case is a separate implementation since it's async and needs to rely on callbacks. The sync implementation of caching middleware is simpler because the Response it gets from app.call(env) is guaranteed to already be finalized. This is an unfortunate product of Faraday's bad support for parallelism and hackish way for Faraday::Response objects to be restored from storage as if they've been constructed from a real response. I'm not sure how to properly test it, but once we have good tests for async cases, we could try consolidating the implementation so we don't have separate execution branches for these modes of operation.

@jjb
Copy link
Contributor Author

jjb commented Oct 7, 2014

Are we sure that users are using this feature?

Perhaps we should at least document it, or point to an article or something describing its use.

Still seems smelly to me, like this coordination should be the responsibility of a layer higher up.

@mislav
Copy link
Contributor

mislav commented Oct 7, 2014

On Tue, Oct 7, 2014 at 5:19 PM, John Bachir notifications@github.com
wrote:

Are we sure that users are using this feature?

Adapters like EventMachine and Typhoeus support parallelism, so I think
some Faraday users are doing that, yeah.

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

No branches or pull requests

2 participants