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

Work around file size issues at 32 Bit systems #74

Merged
merged 5 commits into from
Jun 12, 2017

Conversation

derkostka
Copy link
Contributor

Replace stream_copy_to_stream with a chunked process, to handle file sizes hat do not fit into an integer.
Implementation according to http://php.net/manual/en/function.stream-copy-to-stream.php#98119, proposed and implemented by https://github.com/rikmeijer in nextcloud/server#1707 (comment).

Replace stream_copy_to_stream with a chunked process, to handle file sizes hat do not fit into an integer.
Implementation according to http://php.net/manual/en/function.stream-copy-to-stream.php#98119, proposed and implemented by https://github.com/rikmeijer in nextcloud/server#1707 (comment).
@staabm
Copy link
Member

staabm commented Apr 9, 2017

Filed a php bug https://bugs.php.net/bug.php?id=74395

@derkostka
Copy link
Contributor Author

derkostka commented Apr 23, 2017

@evert what do you think of that 'workaround' ?
The topic was now discussed here: https://bugs.php.net/bug.php?id=74395, but it seems like this won´t be changed. Performance wise, it may be slower, but 100% working on 32 Bit Systems would be on the pro side.

@staabm
Copy link
Member

staabm commented Apr 23, 2017

I guess we could also detect 32 bit builds and use the workaround only there. That way 64bit builds would not suffer from this workaround (perf wise)

@derkostka
Copy link
Contributor Author

Good idea, relating to PHP_INT_SIZE. I would not mind it being (if even noticeable) slower on 32 Bit.

@derkostka
Copy link
Contributor Author

derkostka commented Jun 11, 2017

Is there any plan to merge this update ? what do you think ?

Otherwise please close.

@derkostka
Copy link
Contributor Author

solves:
owncloud/core#23788
and
nextcloud/server#1707

@staabm
Copy link
Member

staabm commented Jun 11, 2017

Please make the workarround 32bit only and I will merge this PR

@derkostka
Copy link
Contributor Author

Thanks a lot !

I updated the request accordingly:
059b680

@staabm staabm merged commit 45c6e88 into sabre-io:master Jun 12, 2017
staabm pushed a commit that referenced this pull request Jun 12, 2017
@staabm
Copy link
Member

staabm commented Jun 12, 2017

merged and release 4.2.3

@derkostka
Copy link
Contributor Author

Thank you!

} else {
// workaround for 32 Bit systems to avoid stream_copy_to_stream
while (!feof($body)) {
fwrite($output, fread($body, 8192));
Copy link
Member

Choose a reason for hiding this comment

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

fwiw, 8192 seems low. I think you should push blocks 1MB at a time. It's not the 80's anymore ;)

Copy link
Member

Choose a reason for hiding this comment

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

as this path is used for 32bit php its a bit like php from the 80's :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants