Streaming requests for Net::HTTP #604

nightscape · 2016-08-30T21:40:34Z

This PR is a rebased version of #461 and #522 including an additional test and fix for empty initial chunks mentioned by @grosser in #461 (comment).
Tests are all green on my machine, so it would be great if someone (maybe @mislav or @iMacTia) could have a look at this 😃
The other two PRs can be closed in favor of this one.

grosser · 2016-08-30T21:48:56Z

lib/faraday/adapter/em_synchrony.rb

@@ -26,6 +26,11 @@ def call(env)
        # Queue requests for parallel execution.
        if env[:parallel_manager]
          env[:parallel_manager].add(request, http_method, request_config(env)) do |resp|
+            if (req = env[:request]).stream_response?


extract into a method to avoid duplication ?

I'm running in circles trying to find a nice way to extract a method here.
Do you have something in mind?

nope ... could put it on the request or make it static ...

On Tue, Aug 30, 2016 at 3:11 PM, Martin Mauch notifications@github.com
wrote:

In lib/faraday/adapter/em_synchrony.rb
#604 (comment):

@@ -26,6 +26,11 @@ def call(env)
# Queue requests for parallel execution.
if env[:parallel_manager]
env[:parallel_manager].add(request, http_method, request_config(env)) do |resp|

if (req = env[:request]).stream_response?

I'm running in circles trying to find a nice way to extract a method here.
Do you have something in mind?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/lostisland/faraday/pull/604/files/60cdd853545a06a19fd9cd771bbace1196cea141#r76889487,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAAsZz0RQFxpvFbCQALEXeVix6izqDMnks5qlKp7gaJpZM4JxAhc
.

I'm not 100% sure of it as well, but here are my 2 cents.
I personally like having a perform_request method on most of the adapters. I would suggest having a perform_stream_request method to manage stream logics and having the call method calling the proper "perform" based on a check with stream_response?.

The common logic will therefore be something like (extremely simplified):

def call(env) if req.stream_response? perform_stream_request else perform_request end end

At this point, perform_stream_request can be implemented in the common Faraday::Adapter triggering the warning message and override this method only in the adapters where
you want to provide a proper implementation.
I really hope this is clear, but please do let me know if that's not the case!

Good work btw 👍!

I still can't find a nice way to make this work.
If the call method you sketched above was located in Faraday::Adapter then sub-classes would need to have the same signature for perform_request which doesn't seem to be the case.
The same applies to perform_stream_request which - if I understood correctly - should delegate to perform_request unless overridden in a sub-class.
The only way I can currently think of to make this work is to copy the above code snippet into the Adapter sub-classes and have individual implementations of perform_streaming_request which would have a similar signature as perform_request.
But maybe I'm just overlooking something.

Would you be OK with merging even in the current unDRY state of the code?
Maybe someone comes up with a nicer solution later, I'm currently not seeing a proper way out.

I suppose that's ok if we make tests work so we don't break everything later refactoring the code 😁

Travis tests are working now 👍

nightscape · 2016-08-30T21:52:36Z

Hmm, I'm rather clueless why Travis is failing. Tests are all green, but there are some cryptic warning: previous definition of xxxxx was here syntax warnings here https://travis-ci.org/lostisland/faraday/jobs/156358909#L216 and a lot of

ERROR WEBrick::HTTPStatus::ProxyAuthenticationRequired

at the end of the build.

nightscape · 2016-08-31T06:48:32Z

Giving this a little more thought, I think a much nicer API would be a Faraday::Response#chunks method which returns an Enumerator. That way, you could achieve the same as with a callback using each on the Enumerator, but you could also map, select, ... on it and pass the Enumerator around like any object.
Trying this now...

iMacTia · 2016-08-31T14:05:36Z

@nightscape first of all thanks for this important contribution!
I've never had the need to use Faraday to read from a streaming so I'm not sure I will be able to contribute on this, but surely will come useful for future users :)

I've already commented on the repetition of the code with the warning with my suggestion to limit that issue and provide a common interface on all adapters (checking stream_response? on the call method and then delegating to the proper method).

Regarding the failing tests, I'll need a computer to see what's wrong and I won't have one in the next couple of weeks. @sbleon I suppose this is not the same issue you fixed last day?

Finally, the Enumerator seems also a good idea, but there's still something I don't really get...
Since we're talking about a stream, this means that the connection will be kept open and new chunks will be added to the response over time, so the response could potentially never end.
We should design this so that people can easily use it in most situations.

Just to make this easier to understand, can you show a couple of example on how you would use this streaming functionality?

sbleon · 2016-08-31T14:47:16Z

test/live_server.rb

@@ -1,4 +1,5 @@
 require 'sinatra/base'
+load 'test/helper.rb'


I'm guessing the ruby syntax warnings about re-declarations are due to this line, as some of the things it's complaining about are defined in test/helper.rb. I'm pretty sure this test helper script shouldn't try to include the setup for the test suite itself.

I'm not sure why you have this load here at all. Is it because you want to use Faraday::TestCase.big_string? If so, you can just copy that method over to Faraday::LiveServer. I think it's too small to worry about the duplication.

Good point, if it is complaining is probably because that file gets loaded twice. Maybe changing load with require will fix the issue at requiring the same file twice only loads it once

That might do it, but I still think you should bring in only what you need.
Maybe big_string could be moved into a module that can be used by both
test/helper and live_server?

On Wed, Aug 31, 2016 at 1:11 PM, Mattia notifications@github.com wrote:

In test/live_server.rb
#604 (comment):

@@ -1,4 +1,5 @@
require 'sinatra/base'
+load 'test/helper.rb'

Good point, if it is complaining is probably because that file gets loaded
twice. Maybe changing load with require will fix the issue at requiring
the same file twice only loads it once

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/lostisland/faraday/pull/604/files/f0ea17749ae5bb5777818efc0fa7afe711ecb7ae#r77029751,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAMIN115Z8_xL9DT5wfXQNOeZ6tJRTzgks5qlbW5gaJpZM4JxAhc
.

Makes sense to me 😄
Any DRY solution is welcome!

nightscape · 2016-09-05T21:41:20Z

I've pondered the Enumerator idea again, and I think it has one fundamental flaw: Control over the reading the response is given to the user of the Enumerator. Endless responses would not be so much of an issue (that's one of the typical use cases of Enumerators), but if a client just called first on the Enumerator and kept the Enumerator instance around it might be that the response and the connection remain open indefinitely (or until some timeout cuts the connection).
I might be wrong on this, but maybe it's better to continue with the original callback style which should not have this issue.

I've incorporated @sbleon's proposal of extracting the big_string method into a shared module, so except for the duplication of warning message creation (for which I can't find a good solution right now) this would be good to merge from my side.

What do you think, @iMacTia and @sbleon?

iMacTia · 2016-09-06T14:32:53Z

How is the documentation? Do we have a good explanation on how to use te callback to manage the streaming request?

Apologies again for not checking myself but doing everything with the mobile is kind of painful 😥

nightscape · 2016-09-06T21:01:48Z

I just added a small example to the README.
Is there another place where documentation should be added?

iMacTia · 2016-09-07T13:18:28Z

Not as far as I know, README should be fine 😁
I'm ok with merging at this point. if @sbleon agrees as well, he can proceed 👍

akshaylive · 2016-09-15T22:30:32Z

Good PR! Looking forward to this one! 👍

iMacTia · 2016-09-16T01:37:15Z

@nightscape wait a sec, are warning still there even though you followed @sbleon advice? Did you remove the load test/helper.rb after extracting the big_string method?

nightscape · 2016-09-16T08:08:17Z

Hi @iMacTia, the warnings due to big_string are solved.
What I was referring to above is that the code that generates warnings for adapters that don't support streaming is still duplicated because I haven't found a nice way to DRY it.
So from my side this is good to merge and maybe later someone comes up with a nice idea of how to DRY or improve the code.

iMacTia · 2016-09-16T13:25:56Z

I see, sorry @nightscape, reading again after some days I confused the build warnings with those ones 😅
So again, all good for me if @sbleon agreed as well. If he's still busy and can't merge, don't worry, I will merge early next as soon as I have a proper computer to check it 😉

nurse · 2016-09-20T01:13:01Z

lib/faraday/adapter/net_http.rb

+            if chunk.size > 0 || size > 0
+              yielded = true
+              size += chunk.bytesize
+              env[:request].on_data.call(chunk, size)


In README.md it says the call's 2nd argument is chunk_size, but size here is total size.
Which do you intend?

Good catch :) Which one would be preferred? Bytes or Unicode characters? I don't care either way.

I think what the documentation states is probably the best option in this case:

Set a callback which will receive tuples of chunk Strings
and the sum of characters received so far

as the chunk_size can be easily obtained calling chunk.bytesize, making it quite useless as parameter.
The only "wrong" thing here, I think, is the documentation name for the block parameter.
Maybe size rather than chunk_size will remove the confusion :)

@nightscape It must be bytes because HTTP response body may be other than Unicode or simple binary data.

@iMacTia yeah, I agree. just fixing README sounds reasonable.

nurse · 2016-09-20T01:13:37Z

lib/faraday/adapter/net_http.rb

+          size = 0
+          yielded = false
+          http_response = perform_request_with_wrapped_block(http, env) do |chunk|
+            if chunk.size > 0 || size > 0


chunk.size should be chunk.bytesize.

nurse · 2016-09-20T01:14:46Z

test/adapters/integration.rb

+
+        # Check that the total size of the chunks (via the last size returned)
+        # is the same size as the expected_response
+        assert_equal sizes.last, expected_response.size


expected_response.size should be expected_response.bytesize.

iMacTia · 2016-09-27T10:55:19Z

@nightscape: @nurse highlighted a couple of things that are not completely clear, do you think you can have a look at them before we proceed? Thanks

nightscape · 2016-10-04T17:43:20Z

@nurse: I added two fixup commits. If those changes are what you meant and Travis gives us green light, I'll squash them into their corresponding commits.
I also rebased upon the latest master version.

nightscape · 2016-11-26T23:17:44Z

Hi @iMacTia,

that sounds reasonable. We'll be using our own fork or maybe just wait for 1.0 👍

iMacTia · 2017-08-18T14:51:20Z

@nightscape Coming back to this as part of the v1.0 works.
Have you been using this for the past months? Is there any change you would like to add before this gets merged?
Would you be able to rebase against v1.0 branch? (please note I've changed the PR base branch already

nightscape · 2017-08-26T08:24:20Z

Hi @iMacTia,

because of shifting priorities and a partial move to Scala we haven't been using the code in the past months.
I can try to rebase the PR against 1.0, let's see if I still find my way around 😉

@bmaddy

Based on work by @bmaddy, @james-lawrence and @ANorwell https://github.com/backupify/faraday/tree/streaming-0.9.0 https://github.com/bmaddy/faraday/tree/streaming

Under certain conditions, it seems that Net::HTTP first returns an empty chunk before reading the actual data. This commit makes sure that such chunks are filtered out for the case where later chunks contain data. If the response is completely empty, an empty chunk is returned nonetheless so that clients can safely assume that their callback will be called at least once.

nightscape · 2017-08-29T16:20:41Z

Hi @iMacTia,
I just rebased the code (there was only one minor conflict in option handling).
Travis still says everything's good 😃

iMacTia · 2017-08-30T07:09:41Z

Hi @nightscape, thank you very much!
I really appreciate you finding some time to come back to this!
It was an highly requested feature so I'm happy to make this the first important merge of v1.0 👍
We'll probably need to do some refinement, but this looks like a great start!
Thanks

nightscape · 2017-08-30T08:37:19Z

Thank you for looking into it and merging!

grosser · 2017-08-30T14:27:49Z

amazing, it finally happened :D

iMacTia · 2017-08-30T14:38:36Z

I never said this wouldn't happen 😉
I'm just sorry this took so long.
@grosser are you still interested in using this? It would be nice to have some real-world testing of these changes. Maybe not in production but on a side project 😄

grosser · 2017-08-30T14:52:17Z

no idea where/why I needed that :D

This was referenced Aug 30, 2016

Streaming requests for Net::HTTP #461

Closed

Streaming requests for Net::HTTP #522

Closed

nightscape changed the title ~~Streaming implementation for Net::HTTP~~ Streaming requests for Net::HTTP Aug 30, 2016

nightscape force-pushed the streaming branch from 510bb32 to 60cdd85 Compare August 30, 2016 21:47

grosser reviewed Aug 30, 2016
View reviewed changes

nightscape force-pushed the streaming branch from 60cdd85 to f0ea177 Compare August 30, 2016 21:57

sbleon reviewed Aug 31, 2016
View reviewed changes

nightscape force-pushed the streaming branch 3 times, most recently from 3dda408 to d216179 Compare September 5, 2016 21:24

nurse reviewed Sep 20, 2016

View reviewed changes

nightscape force-pushed the streaming branch 2 times, most recently from 9da67fd to 6b984fd Compare October 4, 2016 17:37

iMacTia self-assigned this Nov 8, 2016

iMacTia added this to the 1.0 milestone Nov 8, 2016

iMacTia mentioned this pull request Nov 8, 2016

Streaming #156

Closed

nightscape closed this Nov 26, 2016

iMacTia mentioned this pull request Feb 7, 2017

Request: Allow setting of options of underlying http libraries #661

Closed

cben mentioned this pull request Mar 26, 2017

switch http client to something that does not have a native extension ManageIQ/kubeclient#237

Open

patrickvinograd mentioned this pull request Apr 24, 2017

[WIP] health records streaming client department-of-veterans-affairs/vets-api#931

Closed

escowles mentioned this pull request Jul 13, 2017

Fedora adapter(s) should support streaming binary content samvera/valkyrie#145

Open

iMacTia changed the base branch from master to 1.0 August 18, 2017 12:53

iMacTia reopened this Aug 18, 2017

Version bump (0.13.1)

29feeb9

james-lawrence and others added 3 commits August 29, 2017 17:10

Streaming requests for Net::HTTP

864710e

Based on work by @bmaddy, @james-lawrence and @ANorwell https://github.com/backupify/faraday/tree/streaming-0.9.0 https://github.com/bmaddy/faraday/tree/streaming

Add streaming example to README

163f5c0

vinidourado87 force-pushed the streaming branch from e50eee8 to 163f5c0 Compare August 29, 2017 15:17

iMacTia removed the unconfirmed label Aug 30, 2017

iMacTia merged commit ff94676 into lostisland:1.0 Aug 30, 2017

nightscape deleted the streaming branch August 30, 2017 08:36

zgalor mentioned this pull request Jan 30, 2018

Include #604 in a release #764

Closed

iMacTia mentioned this pull request Feb 2, 2018

Advice on how to implement adapter #766

Closed

ghost mentioned this pull request Dec 12, 2019

Add a follow parameter for client/fs/stream/:alloc_id endpoint bigcommerce/nomad_client#7

Merged

Streaming requests for Net::HTTP #604

Streaming requests for Net::HTTP #604

Conversation

nightscape commented Aug 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nightscape commented Aug 30, 2016

nightscape commented Aug 31, 2016

iMacTia commented Aug 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nightscape commented Sep 5, 2016

iMacTia commented Sep 6, 2016

nightscape commented Sep 6, 2016

iMacTia commented Sep 7, 2016

akshaylive commented Sep 15, 2016

iMacTia commented Sep 16, 2016

nightscape commented Sep 16, 2016

iMacTia commented Sep 16, 2016

nurse Sep 20, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iMacTia Sep 27, 2016 • edited

Choose a reason for hiding this comment

nurse Sep 30, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iMacTia commented Sep 27, 2016

nightscape commented Oct 4, 2016

nightscape commented Nov 26, 2016

iMacTia commented Aug 18, 2017

nightscape commented Aug 26, 2017

nightscape commented Aug 29, 2017

iMacTia commented Aug 30, 2017

nightscape commented Aug 30, 2017

grosser commented Aug 30, 2017

iMacTia commented Aug 30, 2017

grosser commented Aug 30, 2017

nurse Sep 20, 2016 •

edited

iMacTia Sep 27, 2016 •

edited

nurse Sep 30, 2016 •

edited