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

Use Map::computeIfAbsent to simplify scope implementations #25038

Merged
merged 1 commit into from May 10, 2020

Conversation

quaff
Copy link
Contributor

@quaff quaff commented May 9, 2020

It's a tiny improvement.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label May 9, 2020
@quaff quaff changed the base branch from master to 5.2.x May 9, 2020 07:53
@sbrannen sbrannen self-assigned this May 10, 2020
@sbrannen sbrannen added in: core Issues in core modules (aop, beans, core, context, expression) type: task A general task and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels May 10, 2020
@sbrannen sbrannen added this to the 5.3 M1 milestone May 10, 2020
@sbrannen sbrannen changed the title Use Map::computeIfAbsent to simplify code Use Map::computeIfAbsent to simplify scope implementations May 10, 2020
@sbrannen sbrannen merged commit 50a4fda into spring-projects:5.2.x May 10, 2020
@sbrannen
Copy link
Member

sbrannen commented May 10, 2020

This has been merged into 5.2.x and master.

Thanks

@sbrannen sbrannen modified the milestones: 5.3 M1, 5.2.7 May 10, 2020
FelixFly pushed a commit to FelixFly/spring-framework that referenced this pull request Aug 16, 2020
sbrannen added a commit that referenced this pull request Aug 22, 2020
PR gh-25038 introduced regressions in SimpleThreadScope and
SimpleTransactionScope in Spring Framework 5.2.7. Specifically, if a
thread-scoped or transaction-scoped bean has a dependency on another
thread-scoped or transaction-scoped bean, respectively, a
ConcurrentModificationException will be thrown on Java 11 or higher.

The reason is that Java 11 introduced a check for concurrent
modification in java.util.HashMap's computeIfAbsent() implementation,
and such a modification can occur when a thread-scoped bean is being
created in order to satisfy a dependency of another thread-scoped bean
that is currently being created.

This commit fixes these regressions by switching from HashMap to
ConcurrentHashMap for the instance maps in SimpleThreadScope and
SimpleTransactionScope.

Closes gh-25618
@sbrannen
Copy link
Member

@quaff, for future reference, please note that this PR introduced regressions (see #25618).

sbrannen added a commit that referenced this pull request Sep 25, 2020
Issues gh-25038 and gh-25618 collectively introduced a regression for
thread-scoped and transaction-scoped beans.

For example, given a thread-scoped bean X that depends on another
thread-scoped bean Y, if the names of the beans (when used as map keys)
end up in the same bucket within a ConcurrentHashMap AND an attempt is
made to retrieve bean X from the ApplicationContext prior to retrieving
bean Y, then the use of Map::computeIfAbsent in SimpleThreadScope
results in recursive access to the same internal bucket in the map.

On Java 8, that scenario simply hangs. On Java 9 and higher,
ConcurrentHashMap throws an IllegalStateException pointing out that a
"Recursive update" was attempted.

In light of these findings, we are reverting the changes made to
SimpleThreadScope and SimpleTransactionScope in commits 50a4fda and
148dc95.

Closes gh-25801
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) type: task A general task
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants