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] make XmlEncoder stateless thus reentrant#38534

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

@connorhu
Copy link
Contributor

QA
Branch?5.x
Bug fix?yes
New feature?no
Deprecations?no
TicketsFix#37354
LicenseMIT

Changing dom property of XmlEncoder to stackable. It fixes a DOMException "Wrong document error". When you calling XmlEncoder->encode() in parallel createDomDocument replaces the DomDocument object.
Test code:
https://github.com/connorhu/symfony-serializer/blob/master/test.php

@connorhuconnorhuforce-pushed theserializer/stackable-xml-encoder branch from6b27d66 to39615f4CompareOctober 12, 2020 20:30
@connorhuconnorhu changed the titlechanged dom property to pop/pushable[serializer ]changed dom property to pop/pushableOct 12, 2020
@connorhuconnorhu changed the title[serializer ]changed dom property to pop/pushable[serializer] changed dom property to pop/pushableOct 12, 2020
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

can you please add a test case and rebase+target the lowest branch where this issue exists (3.4 I suppose?)

connorhu reacted with thumbs up emoji
@nicolas-grekasnicolas-grekas added this to the3.4 milestoneOct 13, 2020
@stof
Copy link
Member

Rather than maintaining a stack of current document (which is not properly maintained in case of errors btw, as the unstacking should be done in afinally block), a better solution could be to avoid the need for the state at all, by removing$this->dom. Instead, the\DOMDocument could be passed as argument to private methods needing it (and a bunch of the existing private methods might use$node->ownerDocument instead of$this->dom to avoid the need to add an argument in their signature).

@connorhu
Copy link
ContributorAuthor

@stof I have tested the ownerDocument method. It seems to be a working solution and the tests are running but. At the appendNode method the $parentNode argument can be DomDocument so the onwerDocument is null:
connorhu@a6f0be3#diff-b031dbb2186367839adfe20696612866da91513dd315f00ad6df23b5d38fca74L467-R452

@stof
Copy link
Member

@connorhu if the parentNode is already a DomDocument, you already have the DomDocument. That's fine to me.

@connorhuconnorhuforce-pushed theserializer/stackable-xml-encoder branch from39615f4 to06ac911CompareOctober 13, 2020 13:39
@stof
Copy link
Member

If we want to make the encoder re-entrant, I think the state in$this->context is also an issue, not only$this->dom

@connorhuconnorhuforce-pushed theserializer/stackable-xml-encoder branch from06ac911 toa8c344fCompareOctober 13, 2020 14:10
@nicolas-grekasnicolas-grekas changed the base branch from5.x to3.4October 14, 2020 16:34
@nicolas-grekasnicolas-grekas changed the title[serializer] changed dom property to pop/pushable[Serializer] removed dom property from XmlEncoderOct 14, 2020
@nicolas-grekasnicolas-grekasforce-pushed theserializer/stackable-xml-encoder branch 2 times, most recently fromb58889b to85a02f5CompareOctober 14, 2020 16:37
@nicolas-grekas
Copy link
Member

I rebased the PR for branch 3.4 but there are some failure to fix, could you please have a look?

@connorhu
Copy link
ContributorAuthor

@nicolas-grekas Of course! Thank you!

@connorhuconnorhuforce-pushed theserializer/stackable-xml-encoder branch from89852c4 todb628e6CompareOctober 14, 2020 19:27
@connorhu
Copy link
ContributorAuthor

I think my original bug is now fixed. The current version of XmlEncoder in symfony 3.4 is not so complex. I don't see how can cause the context property a bug like the dom did.

@stof
Copy link
Member

@connorhu the issue is exactly the same that with$this->dom actually: if you call the serializer again during serialization, sotring the call-specific state in the instance will break as the encoder won't be re-entrant (it handles only one state at a time), and it might end up using the wrong state.
The reason you don't see an issue in your reproducing code is because you use an empty context for both calls, so using the wrong empty context is not an issue.

@connorhu
Copy link
ContributorAuthor

@stof I see the possibility of the context bug, but If you use nothing from context after initialization it can't cause a bug. Safer to remove context property instead of making a note for the future code changes.

@stof
Copy link
Member

@connorhu the thing is, the XmlEncoderis using the context in some of the private methods.

@connorhu
Copy link
ContributorAuthor

@stof Should I merge the context property remove patch into in this PR? The referred bug only describes the bug about the dom property.

@fabpotfabpot modified the milestones:3.4,4.4Oct 28, 2020
@nicolas-grekasnicolas-grekas changed the title[Serializer] removed dom property from XmlEncoder[Serializer] make XmlEncoder stateless thus reentrantJan 28, 2022
@nicolas-grekasnicolas-grekas changed the base branch from3.4 to4.4January 28, 2022 10:10
@nicolas-grekasnicolas-grekasforce-pushed theserializer/stackable-xml-encoder branch fromdb628e6 to693f9abCompareJanuary 28, 2022 10:10
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

(I rebased and implemented the remaining)

@nicolas-grekas
Copy link
Member

Thank you@connorhu.

@nicolas-grekasnicolas-grekas merged commit5c67f40 intosymfony:4.4Jan 31, 2022
This was referencedFeb 28, 2022
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@dunglasdunglasAwaiting requested review from dunglasdunglas is a code owner

@lyrixxlyrixxAwaiting requested review from lyrixx

@srozesrozeAwaiting requested review from sroze

@xabbuhxabbuhAwaiting requested review from xabbuh

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

[Serializer] paralell XMLEncoder::encode can cause "Wrong Document Error"

6 participants

@connorhu@stof@nicolas-grekas@fabpot@derrabus@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp