Skip to content

Defaulting the JPA bootstrap mode to deferred may result in deadlock #23758

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

Closed
snicoll opened this issue Oct 20, 2020 · 6 comments
Closed

Defaulting the JPA bootstrap mode to deferred may result in deadlock #23758

snicoll opened this issue Oct 20, 2020 · 6 comments
Labels
status: declined A suggestion or change that we don't feel we should currently apply type: bug A general bug

Comments

@snicoll
Copy link
Member

snicoll commented Oct 20, 2020

In #16230 we've switched the default JPA bootstrap mode to deferred and have seen a number of various deadlocks related reports since.

Recently we've fixed an issue where datasource initialisation actually happens in a totally separated thread (#22852). Tracing back the history of this issue, this was requested as the deferred mode didn't make a significant difference in terms of startup time (see #14061).

We've reworked that pattern based on recommendations of the core spring framework team. Taking a step back and looking at the feature in the core container, we don't think it is warranted to have it the default anymore and we've decided to change the default JPA bootstrap mode to default.

@snicoll snicoll added the type: enhancement A general enhancement label Oct 20, 2020
@snicoll snicoll added this to the 2.3.5 milestone Oct 20, 2020
@snicoll snicoll changed the title Switch JPA bootstrap to default Switch JPA bootstrap mode to default Oct 20, 2020
@snicoll snicoll added type: bug A general bug and removed type: enhancement A general enhancement labels Oct 20, 2020
@wilkinsona wilkinsona changed the title Switch JPA bootstrap mode to default Defaulting the JPA bootstrap mode to deferred may result in deadlock Oct 20, 2020
@wilkinsona
Copy link
Member

wilkinsona commented Oct 20, 2020

FYI, @odrotbohm, as you raised the original issue.

Within the constraints of Framework 5.2 (and most probably 5.3) it isn't possible to use deferred initialization and be sure that a deadlock won't occur. We prefer to prioritise stability and predictability over speed so, unfortunately, we need to switch back to using the default bootstrap mode by default. Users can then opt into the deferred mode if it works in their specific scenario.

@odrotbohm
Copy link
Member

Is it really the delayed repository initialization that's causing the issue or is it the asynchronous EMF bootstrap that's triggered by the switch as well?

@snicoll
Copy link
Member Author

snicoll commented Oct 20, 2020

It's rather the extra care that you need to apply to everything that may access the EntityManagerFactory on startup. The last occurence we had was an auto-configuration that's reporting Hibernate metrics and was doing so in an @Autowired method. We've had to push this down to a SmartInitializingBean.

The framework at this point does not provide a programmatic model where that sort of things can be done "naturally" so it is very easy to do it wrong. We don't expect a lot of users to play with the EMF in bootstrap code but that alone justifies to make it opt-in.

@jhoeller
Copy link

jhoeller commented Oct 20, 2020

The main risk here is a blocking EntityManagerFactory call in in the bootstrap thread within the singleton lock. This is effectively the case within any @Autowired injection method and also within InitializingBean.afterPropertiesSet but not within SmartInitializingSingleton.afterSingletonsInstantiated which runs in a separate phase.

On its own, such a blocking EMF call is unpleasant (since it prevents the main bootstrap thread from making other progress) but not necessarily causing a deadlock. Only in combination with singleton bean creation triggered by an operation in the EMF bootstrap thread, it's bound to run into the deadlock. Pre-initializing all affected singleton beans (including all event listeners) therefore helps as well, but that's hard to consistently enforce. From that perspective, I'd rather make the user opt-in explicitly.

@odrotbohm
Copy link
Member

Yeah, I was sort of assuming stuff like that. I just wanted to make sure there's nothing we can actually do from the Spring Data side to address this in the deferring mechanism. In fact, that mechanism is a workaround to prevent immediate interaction with the EMF in the first place.

@snicoll snicoll added the status: noteworthy A noteworthy issue to call out in the release notes label Oct 20, 2020
@wilkinsona
Copy link
Member

With thanks to @odrotbohm for the discussion and suggestions, we have an alternative fix for #22852 that means that we can leave the default as-is.

@wilkinsona wilkinsona removed this from the 2.3.5 milestone Oct 28, 2020
@wilkinsona wilkinsona added status: declined A suggestion or change that we don't feel we should currently apply and removed status: noteworthy A noteworthy issue to call out in the release notes labels Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: declined A suggestion or change that we don't feel we should currently apply type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants