Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix concurrency issues in XStreamMarshaller #25017

Closed
JakobFels opened this issue May 5, 2020 · 3 comments
Closed

Fix concurrency issues in XStreamMarshaller #25017

JakobFels opened this issue May 5, 2020 · 3 comments
Assignees
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) status: backported An issue that has been backported to maintenance branches type: enhancement A general enhancement
Milestone

Comments

@JakobFels
Copy link

JakobFels commented May 5, 2020

Just stumbled on a nasty concurrency issue in org.springframework.oxm.xstream.XStreamMarshaller.

When you forget to call afterPropertiesSet on this marshaller, the internally held XStream instance will not be initialized. Instead, it will be done in the method getXStream, which is called from both marshal and unmarshal methods.

This initialization is not guarded, and might happen concurrently. In this case, the ConverterLookup used to store converters will be shared by the XStream instances that are constructed, which can lead to strange application behaviour, since the underlying data structure is not thread safe.

In our application, it lead to very strange errors during XML marshalling/unmarshalling, since a couple of converters were missing. This happened whenever our app process scaled up during high load.

I see 2 possible solutions:

  • throw an exception in case afterPropertiesSet is not called on an instance
  • make XStream initialization thread safe

Spring version affected is 5.2.5.RELEASE

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label May 5, 2020
@sbrannen sbrannen added the in: data Issues in data modules (jdbc, orm, oxm, tx) label May 5, 2020
@sbrannen sbrannen self-assigned this May 5, 2020
@sbrannen sbrannen added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels May 5, 2020
@sbrannen
Copy link
Member

sbrannen commented May 5, 2020

Thanks for raising the issue.

We'd not want to start throwing an exception here, since that might break existing use cases that did not encounter concurrency issues.

Rather, we'll introduce some locking for that field to avoid concurrency issues.

@sbrannen sbrannen added this to the 5.2.7 milestone May 5, 2020
@sbrannen
Copy link
Member

sbrannen commented May 6, 2020

This has been addressed in ce11fdf for the upcoming Spring Framework 5.2.7 release.

Feel free to try it out in the next 5.2.7 snapshot and provide feedback.

@sbrannen
Copy link
Member

sbrannen commented May 7, 2020

Reopening to make use of the SingletonSupplier. See ce11fdf#r39003837 for details.

@sbrannen sbrannen reopened this May 7, 2020
@sbrannen sbrannen added the for: backport-to-5.1.x Marks an issue as a candidate for backport to 5.1.x label May 7, 2020
@spring-projects-issues spring-projects-issues added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-5.1.x Marks an issue as a candidate for backport to 5.1.x labels May 7, 2020
@sbrannen sbrannen changed the title Concurrency issue in XStreamMarshaller Fix concurrency issues in XStreamMarshaller May 7, 2020
sbrannen added a commit that referenced this issue May 7, 2020
Prior to this commit, if an instance of XStreamMarshaller was used
concurrently from multiple threads without having first invoked the
afterPropertiesSet() method, the private `xstream` field could be
initialized multiple times resulting in a ConcurrentModificationException
in XStream's internal DefaultConverterLookup.

This commit fixes these concurrency issues by making the `xstream` field
volatile and by implementing a double-checked locking algorithm in
getXStream() to avoid concurrent instance creation.

Closes gh-25017
FelixFly pushed a commit to FelixFly/spring-framework that referenced this issue Aug 16, 2020
Prior to this commit, if an instance of XStreamMarshaller was used
concurrently from multiple threads without having first invoked the
afterPropertiesSet() method, the private `xstream` field could be
initialized multiple times resulting in a ConcurrentModificationException
in XStream's internal DefaultConverterLookup.

This commit fixes these concurrency issues by making the `xstream` field
volatile and by implementing a double-checked locking algorithm in
getXStream() to avoid concurrent instance creation.

Closes spring-projectsgh-25017
FelixFly pushed a commit to FelixFly/spring-framework that referenced this issue Aug 16, 2020
zx20110729 pushed a commit to zx20110729/spring-framework that referenced this issue Feb 18, 2022
Prior to this commit, if an instance of XStreamMarshaller was used
concurrently from multiple threads without having first invoked the
afterPropertiesSet() method, the private `xstream` field could be
initialized multiple times resulting in a ConcurrentModificationException
in XStream's internal DefaultConverterLookup.

This commit fixes these concurrency issues by making the `xstream` field
volatile and by implementing a double-checked locking algorithm in
getXStream() to avoid concurrent instance creation.

Closes spring-projectsgh-25017
zx20110729 pushed a commit to zx20110729/spring-framework that referenced this issue Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) status: backported An issue that has been backported to maintenance branches type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants