Skip to content

Fix GH-13071: Copying large files using mmap-able source streams may exhaust available memory and fail #13136

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

Closed
wants to merge 3 commits into from

Conversation

nielsdos
Copy link
Member

Commit 5cbe5a5 disabled chunking for all writes to streams. However, user streams have a callback where code is executed on data that is subject to the memory limit. Therefore, when using large writes or stream_copy_to_stream/copy the memory limit can easily be hit with large enough data.

To solve this, we reintroduce chunking for userspace streams. Users have control over the chunk size, which is neat because they can improve the performance by setting the chunk size if that turns out to be a bottleneck.

In an ideal world, we add an option so we can "ask" the stream whether it "prefers" chunked writes, similar to how we have php_stream_mmap_supported & friends. However, that cannot be done on stable branches.

…ay exhaust available memory and fail

Commit 5cbe5a5 disabled chunking for all writes to streams. However,
user streams have a callback where code is executed on data that is
subject to the memory limit. Therefore, when using large writes or
stream_copy_to_stream/copy the memory limit can easily be hit with large
enough data.

To solve this, we reintroduce chunking for userspace streams.
Users have control over the chunk size, which is neat because
they can improve the performance by setting the chunk size if
that turns out to be a bottleneck.

In an ideal world, we add an option so we can "ask" the stream whether
it "prefers" chunked writes, similar to how we have
php_stream_mmap_supported & friends. However, that cannot be done on
stable branches.
@nielsdos nielsdos marked this pull request as ready for review January 13, 2024 00:18
@nielsdos nielsdos requested a review from bukka January 13, 2024 00:18
Copy link
Member

@bukka bukka left a comment

Choose a reason for hiding this comment

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

Could you add a test for the actual mmap issue so it is covered?

@nielsdos
Copy link
Member Author

Could you add a test for the actual mmap issue so it is covered?

Okay I've added a simplified test based on the original code and it seems to pass.

@nielsdos nielsdos closed this in 5e9e9c9 Jan 16, 2024
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.

Copying large files using mmap-able source streams may exhaust available memory and fail
3 participants