search

php-http / artax-adapter

Artax HTTP Adapter
http://httplug.io
MIT License
5 stars 4 forks source link

Stream response bodies instead of always buffering them completely #8

Closed kelunik closed 7 years ago

kelunik commented 7 years ago
Q A
Bug fix? kinda
New feature? kinda
BC breaks? unknown
Deprecations? no
Related tickets Mentioned in #3.
License MIT

What's in this PR?

Use a custom StreamInterface implementation to allow for proper backpressure.

Why?

Large responses have to be held entirely in memory otherwise.

Nyholm commented 7 years ago

Why do you need an anonymous class btw? It will be easier to test if it isn't.

kelunik commented 7 years ago

Doesn't need to be one, but as there's only a single use I just did that.

fbourigault commented 7 years ago

I'm also not a big fan of the anonymous class. It make things harder to read and to test.

kelunik commented 7 years ago

@dbu We can implement streaming for the request pretty easily as long as we keep the "never follow redirects", otherwise we can only stream for rewindable streams, because redirects might require sending the body again.

@dbu @fbourigault What's the name you suggest for that class? Which areas specifically need more tests? IMO everything that's tested should be tested by the common tests, not with individual tests in different clients.

dbu commented 7 years ago

We can implement streaming for the request pretty easily as long as we keep the "never follow redirects", otherwise we can only stream for rewindable streams, because redirects might require sending the body again.

i suggest adding an issue that there is a possible performance improvement of checking if the stream is rewindable and only then copy the request. but not do it now, so we can get a first release ready. its only relevant when sending large requests.

What's the name you suggest for that class? Which areas specifically need more tests? IMO everything that's tested should be tested by the common tests, not with individual tests in different clients.

ArtaxStreamAdapter or something like that? the common tests do not test edge cases of streams i suppose, adding such tests would be good to know exactly when something in the stream is broken, as opposed to "something" with the client somewhere... e.g. when we implement the first point, we can have different tests for rewindable and non rewindable streams and ensure the behaviour is always correct.

Nyholm commented 7 years ago

FYI: There are some unit tests for PSR7 streams in https://github.com/php-http/psr7-integration-tests

kelunik commented 7 years ago

@Nyholm Unfortunately, the stream tests there assume StreamInterface to be tightly coupled to PHP's resources.

What should getContents() return on a second call? According to the PSR document, it returns the remaining contents, but it doesn't say whether they're consumed or just returned. So should the next invocation return the same thing or just an empty string?

Nyholm commented 7 years ago

Hm, maybe they are. We should review that.

What should getContents() return on a second call? According to the PSR document, it returns the remaining contents, but it doesn't say whether they're consumed or just returned. So should the next invocation return the same thing or just an empty string?

It should return an empty string.

kelunik commented 7 years ago

@Nyholm Moved it to a separate class and added tests for full coverage.

dbu commented 7 years ago

cheers!

Nyholm commented 7 years ago

Great work!