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

Use futuroscope to run API calls in the background #30

Merged
merged 6 commits into from
Dec 20, 2013
Merged

Conversation

josepjaume
Copy link
Contributor

I had to change some tests and add an inspect because otherwise the test would have died without letting the thread make its work.

By calling inspect (or any other method, really) we force it to wait for a result.

I believe the speed improvements will be enormous.

@oriolgual
Copy link
Member

It would be nice to have some kind of benchmark :P

@oriolgual
Copy link
Member

Also, I was thinking to enforce the use of NetHTTPPersistent (since it's supported by Faraday) to help reducing the number of connections.

@josepjaume
Copy link
Contributor Author

That would be great!

Let's see if I can run some benchmarks tomorrow :)

@bkeepers
Copy link
Contributor

Could you explain where the speed improvements will come from? I'm not sure I see it. It seems like for most uses, the future will have to resolve for each object before the next one is created. For example, if I wanted to get the titles from an array of links:

Hyperclient.new(url).links[:categories].each {|c| puts c.attributes.name }

c.attributes will create the future, but .name has to wait for it to resolve, which happens before all the requests are created.

Am I missing something?

@txus
Copy link
Member

txus commented Sep 18, 2013

I rebased with current master. @josepjaume maybe you could provide some example code to show where the benefits could be?

@txus
Copy link
Member

txus commented Dec 20, 2013

Merging, but not releasing a new version yet. We'll see if we run into any problems.

txus added a commit that referenced this pull request Dec 20, 2013
Use futuroscope to run API calls in the background
@txus txus merged commit b48dbd7 into master Dec 20, 2013
@txus txus deleted the use-futuroscope branch December 20, 2013 13:27
@langalex
Copy link

I would propose to remove this again:

  1. As stated above there's probably zero performance gain in most cases as the calls all are sequential/depend on each other
  2. It's not apparent when using hyperclient that it uses futures internally, which can lead to problems, i.e. making a POST/PUT request without checking the response -> returns a future -> network problem causes exception but this is ignored as the program has already moved on
  3. Potential problems with non thread-safe code (?)

I think it would be more appropriate to use futures directly in an app where you are aware of the implications.

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

Successfully merging this pull request may close these issues.

5 participants