Skip to content

[Serializer] add caution for normalize Datetime object with GetSetMethodNormalizer #12825

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 22, 2020
Merged

[Serializer] add caution for normalize Datetime object with GetSetMethodNormalizer #12825

merged 1 commit into from
Oct 22, 2020

Conversation

gilles-g
Copy link
Contributor

@gilles-g gilles-g commented Dec 17, 2019 •
edited
Loading

I noticed an issue with the GetSetMethodNormalizer, not a real issue, but just a warning about its use.

If we normalize an object which returns a Datetime, without defining the DatetimeNormalizer; on many objects at the same time, we explode the memory of PHP and time of use.

Example :

$serializer = new Serializer([new GetSetMethodNormalizer()]);
$objects = [];
for ($i = 0, $n = 100;$i<$n; $i++) {
  $objects[] = new MyObject(new \DateTime('now'));
}

$serializer->normalize($objects);

dd('memory_get_peak_usage', (memory_get_peak_usage(true) / 1024 / 1024) . ' MiB');

"memory_get_peak_usage"
"46 MiB"

$serializer = new Serializer([new DateTimeNormalizer(), new GetSetMethodNormalizer()]);
$objects = [];
for ($i = 0, $n = 100;$i<$n; $i++) {
  $objects[] = new MyObject(new \DateTime('now'));
}

$serializer->normalize($objects);

dd('memory_get_peak_usage', (memory_get_peak_usage(true) / 1024 / 1024) . ' MiB');

"memory_get_peak_usage"
"14 MiB"

For i = 1000 => 334 Mib 🌊

@wouterj
Copy link
Member

wouterj commented Oct 21, 2020 •
edited
Loading

Hi @Spike31! A very late thanks to you for creating this PR!

I don't have much knowledge about this topic. Let's ask @dunglas if he can maybe have a look at this PR. Is this a known issue that should be somehow mitigated in the GetSetMethodNormalizer? Or should we add this little caution to the docs instead?

@dunglas
Copy link
Member

dunglas commented Oct 21, 2020

Actually it's the case for all object normalizers, not only this one.

This will also leak a lot of useless internal details (which could even help an attacker).

+1 on my side for a (more general) warning.

@wouterj wouterj force-pushed the serializer_performance_getset branch from 5e789fa to 369ea13 Compare October 22, 2020 11:38
@wouterj
Copy link
Member

wouterj commented Oct 22, 2020

Thanks again Gilles! And thanks for your quick response, Kévin. I've made the caution slightly more general and moved it a bit further up in the article.

@wouterj wouterj merged commit 5016771 into symfony:3.4 Oct 22, 2020
@gilles-g
Copy link
Contributor Author

Thx @wouterj 😀

@gilles-g gilles-g deleted the serializer_performance_getset branch October 13, 2021 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Serializer Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gilles-g @wouterj @dunglas @carsonbot