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

Async::HTTP::Faraday cannot use "in_parallel" #1583

Closed
ioquatix opened this issue Aug 17, 2024 · 6 comments · Fixed by #1584
Closed

Async::HTTP::Faraday cannot use "in_parallel" #1583

ioquatix opened this issue Aug 17, 2024 · 6 comments · Fixed by #1584

Comments

@ioquatix
Copy link
Contributor

We need to wrap the requests with Async{} but the parallel manager is only called AFTER the fact.

https://github.com/lostisland/faraday/blob/3efc0a89825da053e7beb85b9b264f424df0e893/lib/faraday/connection.rb#L324C1-L325C29

In other words, it would be better:

parallel_manager&.run(&block)
@iMacTia
Copy link
Member

iMacTia commented Aug 20, 2024

I'm not a fan myself of the current interface, but as it was pointed out in this comment it is actually possible to deal with this by using the parallel? test method in the adapter, collecting all the requests in an array, and postponing the execution until run is called.

Although I agree with you parallel_manager&.run(&block) would be a much better interface, making this change would break existing implementations 😞 so right now I don't think it's an option for us

@iMacTia iMacTia closed this as completed Aug 20, 2024
@ioquatix
Copy link
Contributor Author

ioquatix commented Aug 20, 2024

We could sniff whether run takes an explicit block:

e.g.

parallel_manager.method(:run).parameters.empty?

That would make it an option to make the proposed change parallel_manager&.run(&block)

Another option is to introduce a different method and use respond_to?.

@iMacTia
Copy link
Member

iMacTia commented Aug 21, 2024

Unorthodox, but it would work.
I'm concerned by the parameters approach though, would rather have a different method and check with respond_to?

@iMacTia iMacTia reopened this Aug 21, 2024
iMacTia added a commit that referenced this issue Aug 21, 2024
The new interface passes the parallel block with all the requests to the ParallelManager, instead of running it beforehand.

This allows for better, stateless ParallelManager implementations.

Fixes #1583
@iMacTia
Copy link
Member

iMacTia commented Aug 21, 2024

@ioquatix take a look at #1584 when you get a chance.
Would you be able to test the new method before I cut a new release?

iMacTia added a commit that referenced this issue Aug 21, 2024
The new interface passes the parallel block with all the requests to the ParallelManager, instead of running it beforehand.

This allows for better, stateless ParallelManager implementations.

Fixes #1583
iMacTia added a commit that referenced this issue Aug 22, 2024
* Add support for a new `ParallelManager#execute` method.

The new interface passes the parallel block with all the requests to the ParallelManager, instead of running it beforehand.

This allows for better, stateless ParallelManager implementations.

Fixes #1583

* Update docs/adapters/custom/parallel-requests.md

Co-authored-by: Olle Jonsson <olle.jonsson@gmail.com>

---------

Co-authored-by: Olle Jonsson <olle.jonsson@gmail.com>
@iMacTia
Copy link
Member

iMacTia commented Aug 26, 2024

@ioquatix FYI – just released Faraday 2.11.0 with support for the new #execute method 👍

@ioquatix
Copy link
Contributor Author

Thanks for working with me on this new feature!

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 a pull request may close this issue.

2 participants