Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Serializer] PreventCannot traverse an already closed generator error by materializing Traversable input#60260

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

santysisi
Copy link
Contributor

QA
Branch?6.4
Bug fix?yes
New feature?no
Deprecations?no
IssuesFix#60141
LicenseMIT

✅ Pull Request description:

This PR addresses the issue reported in the linked ticket, specifically the error:
Cannot traverse an already closed generator

The fix involves convertingTraversable input 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 followingloop already forces all values from the generator into memory:

foreach ($dataas &$value) {$flattened = [];$this->flatten($value,$flattened,$keySeparator,'',$escapeFormulas);$value =$flattened;}

Therefore, materializing the generator withiterator_to_array()does not increase peak memory usage in any meaningful way.
As an example, here's the comparison of peak memory usage (measured withmemory_get_peak_usage) when processing an array of 1 million integers:

  • With the fix: 90,044,272 bytes
  • Without the fix: 89,936,680 bytes

The difference is negligible, confirming that the fix does not introduce a meaningful performance cost in terms of memory.

@carsonbotcarsonbot added this to the6.4 milestoneApr 23, 2025
@carsonbotcarsonbot changed the titleFix: prevent "Cannot traverse an already closed generator" error by materializing Traversable input[Serializer] Fix: prevent "Cannot traverse an already closed generator" error by materializing Traversable inputApr 24, 2025
@nicolas-grekasnicolas-grekas changed the title[Serializer] Fix: prevent "Cannot traverse an already closed generator" error by materializing Traversable input[Serializer] Prevent "Cannot traverse an already closed generator" error by materializing Traversable inputApr 24, 2025
@mtarld
Copy link
Contributor

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.

santysisi reacted with heart emoji

@OskarStarkOskarStark changed the title[Serializer] Prevent "Cannot traverse an already closed generator" error by materializing Traversable input[Serializer] PreventCannot traverse an already closed generator error by materializing Traversable inputApr 30, 2025
@santysisi
Copy link
ContributorAuthor

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! 😄
That said, I believe such a refactor should go into the 7.4 branch, since this PR is just a fix. Also, from what I can tell, the current fix doesn’t significantly impact memory usage, but maybe I’m wrong.

Copy link
Member

@GromNaNGromNaN left a comment
edited
Loading

Choose a reason for hiding this comment

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

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.

dunglas reacted with thumbs up emoji
@santysisisantysisiforce-pushed thefix-csv-encoder-generator-traversal branch fromc6d056d toa159cbdCompareMay 11, 2025 22:13
Copy link
Contributor

@mtarldmtarld left a comment

Choose a reason for hiding this comment

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

@GromNaN, indeed you're right, didn't see about the column names. Then LGTM after a minor suggestion.

santysisi reacted with heart emoji
CSV, $this->encoder->encode($data, 'csv'));
}

public function provideIterable()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
publicfunction provideIterable()
publicstaticfunction provideIterable():iterable

GromNaN, dunglas, and santysisi reacted with thumbs up emojisantysisi reacted with heart emoji
@fabpotfabpotforce-pushed thefix-csv-encoder-generator-traversal branch froma159cbd toc7206a9CompareMay 12, 2025 08:02
@fabpot
Copy link
Member

Thank you@santysisi.

santysisi reacted with heart emoji

@fabpotfabpot merged commit1fb9d17 intosymfony:6.4May 12, 2025
9 of 11 checks passed
This was referencedMay 25, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@GromNaNGromNaNGromNaN left review comments

@fabpotfabpotfabpot approved these changes

@dunglasdunglasdunglas approved these changes

@mtarldmtarldmtarld approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.

7 participants
@santysisi@mtarld@fabpot@dunglas@GromNaN@nicolas-grekas@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp