[Serializer] Prevent Cannot traverse an already closed generator error by materializing Traversable input
#60260
Conversation
|
Hey @santysisi , thanks for this PR! Indeed I'm a bit worried about the memory usage. Could you give it a try with more complex and filled rows? At least, I think this behavior should be opt-in (using a context option) to let applications needing to be really memory efficient work properly. |
Cannot traverse an already closed generator error by materializing Traversable input
|
Hi @mtarld , thanks for your comment 😄 , I really appreciate it! I agree with your concern, and I think it's a really good idea. Yes, I can give it a try with more complex and filled row. I’d be happy to do that if you want! 😄 |
There was a problem hiding this comment.
LGTM after some minor changes.
Indeed I'm a bit worried about the memory usage. Could you give it a try with more complex and filled rows?
At least, I think this behavior should be opt-in (using a context option) to let applications needing to be really memory efficient work properly.
@mtarld the $data is iterated recursively multiple times during the encoding, so all the data must be in-memory to find the column names.
If the iterator is able to re-generate each item on-demand, the memory usage could be lower, but the CPU or network would be more stressed.
src/Symfony/Component/Serializer/Tests/Encoder/CsvEncoderTest.php
Outdated
Show resolved
Hide resolved
c6d056d to
a159cbd
Compare
| CSV, $this->encoder->encode($data, 'csv')); | ||
| } | ||
|
|
||
| public function provideIterable() |
There was a problem hiding this comment.
| public function provideIterable() | |
| public static function provideIterable(): iterable |
…aterializing Traversable input
a159cbd to
c7206a9
Compare
|
Thank you @santysisi. |
✅ Pull Request description:
This PR addresses the issue reported in the linked ticket, specifically the error:
Cannot traverse an already closed generatorThe fix involves converting
Traversableinput into an array usingiterator_to_array($data), in order to allow multiple traversals of generator-based inputs.At first glance, it might seem that this approach significantly increases memory usage, since all generator values are stored in memory. However, this is not the case. In the current logic, the following loop already forces all values from the generator into memory:
Therefore, materializing the generator with
iterator_to_array()does not increase peak memory usage in any meaningful way.As an example, here's the comparison of peak memory usage (measured with memory_get_peak_usage) when processing an array of 1 million integers:
The difference is negligible, confirming that the fix does not introduce a meaningful performance cost in terms of memory.