Skip to content

Conversation

@oleksandr-nc
Copy link

This PR transitions our file download logic from using Guzzle's stream => true option to the more memory-efficient sink option, mirroring improvements made in other integrations (e.g., OneDrive Integration).

While stream => true prevents Guzzle from buffering the entire response body in memory upfront, it returns a Psr\Http\Message\StreamInterface. When we iteratively read from this stream (e.g., with fread()), each chunk is copied into a PHP string variable before being written to disk. This creates temporary data duplication in PHP's userland memory for each chunk.

Furthermore, even with stream => true, data passes through various buffering layers:

  • The operating system's network buffers.
  • PHP's own internal stream buffers.

These, combined with the chunk copied into our PHP variable, contribute to memory pressure during large file downloads, potentially leading to memory_limit issues.

The Guzzle sink option provides a more direct path. Guzzle streams the HTTP response body directly into the provided writable resource (e.g., a file handle) which significantly reduces memory overhead.

@oleksandr-nc oleksandr-nc force-pushed the fix/downloadFile/memory branch from 01c8830 to a0a5430 Compare May 29, 2025 17:26
Signed-off-by: Oleksander Piskun <oleksandr2088@icloud.com>
@oleksandr-nc oleksandr-nc force-pushed the fix/downloadFile/memory branch from a0a5430 to d301e69 Compare May 29, 2025 17:41
@oleksandr-nc
Copy link
Author

oleksandr-nc commented May 29, 2025

Unfortunetly, locally I can not reproduce the error from the CI, for me at my local machine this code works..


Edited: Looking at this issue this PR will make app incompataible(unless Nextcloud maybe added support for sink from that time) with Server Side Encryption. So feel free to close this PR, but at integration_onedrive I will keep these changes as without sink I constantly get memory exceed error.

@marcelklehr
Copy link
Member

mh, looks like we're stuck then, no? either we break sse or we cause OOM. 🤔

@marcelklehr
Copy link
Member

Could we use the sink option to download to a temp file and then stream copy to the encrypted file?

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.

3 participants