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 temp stream instead of string to buffer content #52

Merged
merged 9 commits into from
Dec 23, 2020

Conversation

yookoala
Copy link
Contributor

@yookoala yookoala commented Dec 22, 2020

  • Update MultipartStreamBuilder::build to buffer built content
    in a php://temp stream resource instead of (string) $contents.
    This way, the size of the stream built won't be limited by PHP
    runtime limit.
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Related tickets
Documentation Fully backward compatible without any interface changes. Only internal programming changes.
License MIT

What's in this PR?

Rewrite to append stream contents and boundaries into a "php://temp" stream as a buffer. Contents within predefined limit (2MB) will be handled in memory for speed. Otherwise, PHP will create a temp file for storage. This mean the multipart stream size is virtually without size limit (unless if, of course, your harddisk is full).

Why?

The original MultipartStreamBuilder::build would read everything into a string variable $streams before making it another stream. If the total size of the content exceed the PHP's memory limit, the process would fail.

Checklist

  • Updated CHANGELOG.md to describe BC breaks / deprecations | new feature | bugfix
  • Documentation pull request created (if not simply a bugfix)

Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me. i can imagine that this adds some filesystem overhead for medium sized streams where previously things worked in memory and now go to the temporary file, but if people really need to worry on that level they can configure the temp handling, according to php.net

@GrahamCampbell do you agree or are there any doubts?

@yookoala
Copy link
Contributor Author

yookoala commented Dec 22, 2020

Updated by PR to include mechanism to detect available memory and assign maxmemory to the buffer (size of buffer suitable to stay in memory).

It would subtract from memory_get_usage() result from PHP memory_limit to get the available memory. Then use 1/4 as the maxmemory setup of the "php://temp" stream used for buffering.

I also added a setBufferMaxMemory() public method for overriding this.

* Update MultipartStreamBuilder::build to buffer built content
  in a php://temp stream resource instead of (string) $contents.
  This way, the size of the stream built won't be limited by PHP
  runtime limit.
* Detect available memory (in bytes). Then config the buffer
  stream to stay in memory until reaching 1/4 of that.
* Added setBufferMaxMemory to manually override that limit.
Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is really cool! i think using max 1/4 of available memory by default is a good idea to not use up all the memory with large streams.

src/MultipartStreamBuilder.php Outdated Show resolved Hide resolved
src/MultipartStreamBuilder.php Outdated Show resolved Hide resolved
src/MultipartStreamBuilder.php Outdated Show resolved Hide resolved
@yookoala yookoala force-pushed the buffered-stream branch 2 times, most recently from 5a8f7e5 to 8ae08b7 Compare December 22, 2020 15:45
@yookoala
Copy link
Contributor Author

Finally fixed all the style issues :-)

* Improve documentation to MultipartStreamBuilder::getAvailableMemory
Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cheers, looks good to me. i wait until tomorrow so @Nyholm or @GrahamCampbell get a chance to see it before merging.

@yookoala could you please add a changelog note for this improvement?

src/MultipartStreamBuilder.php Outdated Show resolved Hide resolved
src/MultipartStreamBuilder.php Outdated Show resolved Hide resolved
src/MultipartStreamBuilder.php Outdated Show resolved Hide resolved
src/MultipartStreamBuilder.php Outdated Show resolved Hide resolved
src/MultipartStreamBuilder.php Outdated Show resolved Hide resolved
@GrahamCampbell
Copy link
Contributor

GrahamCampbell commented Dec 22, 2020

I've left some comments. There are some issues, some of which are major (but easy to resolve).

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

I agree that we should use a stream, but lets fallback to PHP default behavior.

src/MultipartStreamBuilder.php Outdated Show resolved Hide resolved
src/MultipartStreamBuilder.php Outdated Show resolved Hide resolved
src/MultipartStreamBuilder.php Outdated Show resolved Hide resolved
src/MultipartStreamBuilder.php Outdated Show resolved Hide resolved
src/MultipartStreamBuilder.php Outdated Show resolved Hide resolved
@GrahamCampbell
Copy link
Contributor

@Nyholm has suggested some excellent changes. 👍

* Remove the memory detection logic from MultipartStreamBuilder::build.
src/MultipartStreamBuilder.php Outdated Show resolved Hide resolved
src/MultipartStreamBuilder.php Outdated Show resolved Hide resolved
* Improve readability of comment.
* Use the old __toString approach if the stream is not isReadable().
Copy link
Contributor

@dbu dbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, thanks!

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool. Im really happy with this now.

Copy link
Contributor

@GrahamCampbell GrahamCampbell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work.

@Nyholm Nyholm merged commit 8819e79 into php-http:master Dec 23, 2020
@yookoala yookoala deleted the buffered-stream branch December 23, 2020 18:11
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.

4 participants