Always store Response in memory

[mvorisek]

Response buffer should always stay in memory for two reasons:

• a) performance
• b) permission and/or read only disk

man https://www.php.net/manual/en/wrappers.php.php

[nicolas-grekas]

Can you back both claims with some data? I never heard about a system without a tmp directory so claim b) looks non-existant to me, and claim a) would need a scenario where this matters, and I've yet to see one. Also, this ignores the memory consumption this change would incur.

~:-1: unless proven otherwise.~

[mvorisek]

The memory consumption should be ok as the code is reached from a string already present in memory. If the source data were not a string (Stream or resource), the code is not reached.

a) performance

This is how I discovered this problem, we have been observing high SSD usage, as our API responses are often above 2MB php://temp treshold.

[nicolas-grekas]

Ah, makes sense now thanks for the insights. I'm wondering if PHP copies the string or if it just references it when writing to the stream. Could you have a look by checking the memory consumption before/after?

[mvorisek]

The data are duplicated.

[nicolas-grekas]

I think it's possible to create a stream wrapper to prevent the duplication. I would take https://www.php.net/manual/en/stream.streamwrapper.example-1.php as a start and instead of relying on globals, I would initialize the buffer via a custom stream context.

But I'm not saying this to block this PR. This could be contributed later.

[nicolas-grekas]

Idea: add a stream context option to the php://memory stream wrapper to initialize the buffer on fopen and prevent the duplication natively. Could you open an issue on php-src so that the idea can be considered?

[mvorisek]

I do not think the data are internally stored in zval. I would say the custom stream wrapper can be a solution to the duplication, but I am not fully sure if it is worth it, as normally the string is build, released, and thus no deduplication is needed.

[GrahamCampbell]

This is quite a breaking change, and will break things for people using low-memory environments, who need to move things to disk.

[mvorisek]

Yes, but as this change affects only input already stored in memory/string, the memory consumption should not be a major BC-break.

[GrahamCampbell]

Fair.

[nicolas-grekas]

I propose https://github.com/Nyholm/psr7/pull/230 instead.

[mvorisek]

closing in favor of https://github.com/Nyholm/psr7/pull/230