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

Add streaming support #59

Merged
merged 4 commits into from
Jun 9, 2020
Merged

Add streaming support #59

merged 4 commits into from
Jun 9, 2020

Conversation

josevalim
Copy link
Contributor

Both HTTP1 and HTTP2 already streamed internally,
this PR simply moves the stream out.

This PR also changed the main Finch API to have a
explicit step that builds the request. Instead of:

Finch.request(MyApp, :get, "/")

You must now do:

Finch.build(:get, "/") |> Finch.request(MyApp)

This is necessary otherwise the Finch.stream/5
API would be very complex. One advantage of this
approach is also that we no longer need to pass
empty headers when configuring get requests, for
example, instead of:

Finch.request(MyApp, :get, "/", [], nil, pool_timeout: 10_000)

One can now write:

Finch.build(:get, "/") |> Finch.request(MyApp, pool_timeout: 10_000)

The previous API has been properly deprecated.

Finally, this PR streamlines the receive_timeout
value. Previously the documentation and each pool
used a different value.

josevalim added 4 commits June 9, 2020 15:42
Both HTTP1 and HTTP2 already streamed internally,
this PR simply moves the stream out.

This PR also changed the main Finch API to have a
explicit step that builds the request. Instead of:

    Finch.request(MyApp, :get, "/")

You must now do:

    Finch.build(:get, "/") |> Finch.request(MyApp)

This is necessary otherwise the `Finch.stream/5`
API would be very complex. One advantage of this
approach is also that we no longer need to pass
empty headers when configuring get requests, for
example, instead of:

    Finch.request(MyApp, :get, "/", [], nil, pool_timeout: 10_000)

One can now write:

    Finch.build(:get, "/") |> Finch.request(MyApp, pool_timeout: 10_000)

The previous API has been properly deprecated.

Finally, this PR streamlines the `receive_timeout`
value. Previously the documentation and each pool
used a different value.
@keathley
Copy link
Collaborator

keathley commented Jun 9, 2020

I've looked through this and everything looks fine to me codewise. I'm going back and forth on the API change. I get the reason to do it and it's probably the best option. I just need to get comfortable with it ;)

@sneako
Copy link
Owner

sneako commented Jun 9, 2020

Very cool! Thank you @josevalim! I'll run the benchmarks at some point today just to confirm there is no significant performance regression.

@keathley
Copy link
Collaborator

keathley commented Jun 9, 2020

Ok, looking over it some more, I'm good with this new API and it should allow for more flexibility in the future. I'm good to merge this unless you wanna do benchmarks first @sneako. I can't imagine there's any substantial perf hit from this.

@sneako
Copy link
Owner

sneako commented Jun 9, 2020

I'm happy to merge as well, thanks again @josevalim !

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.

3 participants