Considered new defaults for JPA repository bootstrap #16230
Comments
spring-projects-issues
added
the
status: waiting-for-triage
|
I wonder if going lazy for When creating a brand new project using initializr, a single test is typically created: This test, as far as I understand, is meant to check that the context loads correctly, and thus that the application won't throw any exception once executed. I understand that this is of course only one test, and that many others should be created, but still: with JPA repositories loaded lazily, this test will pass even though, for example, one of the JPA queries is invalid. And, if DataJpaTest also uses lazy, if just one of the repositories happens not to be tested and is never injected anywhere, then an invalid JPA query in this repository will go unnoticed in tests. Being faster is nice, but when it comes to tests, I'd rather have tests that are a tiny bit slower but that detect bugs. What do you think? |
|
I'm not in favour of any default that increases the risk of deploying something that's broken. Fast startup is all well and good, but I don't think it should be done at the expense of correctness. Other frameworks have made a different choice, and that may turn out to work well for their users, but I think it would be a mistake for us to follow suit purely to speed up startup time or test execution time. Rather than making |
|
What are your thoughts of adding this optimizations as defaults in just spring boot devtools to improve the development experience? |
|
Thanks for the feedback everyone. Very good points included. I guess I've been focussing too much on the how, whereas my actual concern was to make it very easy to make use of these configuration options. I totally agree that we by no means should change the defaults in a way that currently working scenarios (like the general application bootstrap test that @jnizet described) should start to behave different in a less constraining way. The proposal for making Regarding the lazy initialization, I agree that making it the default for DevTools is a good idea. For the test cases, my primary goal was to avoid spending time on initializing beans that are not required by a particular test. For As far as I can see this is currently needed to lazify repositories for such tests: @DataJpaTests
@TestPropertySource(properties = "spring.data.jpa.repositories.bootstrap-mode=lazy")
class MyTests { … }I wonder if this could be shortcutted to an attribute on the |
wilkinsona
added
for: team-attention
|
We're going to:
|
I'm not sure what the In addition, it is not currently possible to have more than one Spring Data repository configured with For these two reasons, we should change the default to |
|
You're right, Scott. Deferring is currently only relevant for JPA as it's the only store for which the bootstrap of the actual persistence mechanism is taking quite a bit of time and is thus parallelized. In fact, we talk about Can you elaborate why you think |
Apologies for the imprecise language. I was referring to the |
scottfrederick
added a commit
to scottfrederick/spring-boot
that referenced
this issue
Jan 22, 2020
Prior to this change, the default BootstrapMode for all auto-configured Spring Data repositories was BootstrapMode.DEFAULT. This commit changes the default BootstrapMode for auto-configured JpaRepositories to BootstrapMode.DEFERRED to allow the initialization of EntityManagerFactory to be parallelized for increased startup efficiency. The default is BootstrapMode.LAZY for tests using @DataJpaTest. Closes spring-projectsgh-16230
philwebb
pushed a commit
that referenced
this issue
Jan 22, 2020
Change the default `BootstrapMode` for auto-configured `JpaRepositories` to `BootstrapMode.DEFERRED` to allow the initialization of `EntityManagerFactory` to be parallelized for increased startup efficiency. Prior to this change, the default BootstrapMode for all auto-configured Spring Data repositories was `BootstrapMode.DEFAULT`. Closes gh-16230
|
Thanks everyone. Happy to see this change applied! 👍 |
|
Hi there. Please is this somewhere documented? We have migrated to spring boot 2.3.0 and our app got in deadlock because of deferred being default. |
|
The deadlock is due to a suspected bug in Spring Framework. The change to the default is mentioned in the release notes. This was added in the last couple of days so it may not have been there if you went looking for it earlier. |
|
This change just cost me several hours of troubleshooting after upgrading our batches to Spring Boot 2.3.0! It works with setting: |
|
@zehnm Sorry for the time it took to track down the problem and thank you for sharing what you learned. This probably needs to be addressed in both Spring Framework (as the need for a default constructor does not appear to be documented) and in Spring Data (to provide a default constructor). I'll bring it to the Data team's attention. |
|
I must have missed the change to the default, but this change affected both the projects I updated. Both intermittently would hang on start up and also when running tests. Changing the bootstrap mode back to |
|
@ctmay4 if you have the time, it would be great if you could capture and share a thread dump when things are deadlocked. |
|
I've filed an issue for Spring Framework and @jhoeller seemed positive to be able to avoid a fix for the needed default constructor with the framework itself. |
|
Here is the thread dump when startup is hanging. |
|
Thanks, @ctmay4, that's very helpful. It looks like issue is caused by the Spring Data web integration being triggered and that trying to look up repositories during initialization and thus ending up waiting to be able to use the JPA meta-model currently in creation. I think |
|
In our case, we are using repositories during application startup |
odrotbohm
added a commit
to spring-projects/spring-data-commons
that referenced
this issue
May 26, 2020
…nverter. DomainClassConverter now delays the initialization of the Repositories instances used to avoid the premature initialization of repository instances as that can cause deadlocks if the infrastructure backing the repositories is initialized on a separate thread. Refactored nested converter classes to allow them to be static ones. Related issues: spring-projects/spring-boot#16230, spring-projects/spring-framework#25131.
mp911de
pushed a commit
to spring-projects/spring-data-commons
that referenced
this issue
May 29, 2020
…nverter. DomainClassConverter now delays the initialization of the Repositories instances used to avoid the premature initialization of repository instances as that can cause deadlocks if the infrastructure backing the repositories is initialized on a separate thread. Refactored nested converter classes to allow them to be static ones. Related issues: spring-projects/spring-boot#16230, spring-projects/spring-framework#25131. Original pull request: #445.
mp911de
pushed a commit
to spring-projects/spring-data-commons
that referenced
this issue
May 29, 2020
…nverter. DomainClassConverter now delays the initialization of the Repositories instances used to avoid the premature initialization of repository instances as that can cause deadlocks if the infrastructure backing the repositories is initialized on a separate thread. Refactored nested converter classes to allow them to be static ones. Related issues: spring-projects/spring-boot#16230, spring-projects/spring-framework#25131. Original pull request: #445.
mp911de
pushed a commit
to spring-projects/spring-data-commons
that referenced
this issue
May 29, 2020
…nverter. DomainClassConverter now delays the initialization of the Repositories instances used to avoid the premature initialization of repository instances as that can cause deadlocks if the infrastructure backing the repositories is initialized on a separate thread. Refactored nested converter classes to allow them to be static ones. Related issues: spring-projects/spring-boot#16230, spring-projects/spring-framework#25131. Original pull request: #445.
mp911de
pushed a commit
to spring-projects/spring-data-commons
that referenced
this issue
May 29, 2020
…nverter. DomainClassConverter now delays the initialization of the Repositories instances used to avoid the premature initialization of repository instances as that can cause deadlocks if the infrastructure backing the repositories is initialized on a separate thread. Refactored nested converter classes to allow them to be static ones. Related issues: spring-projects/spring-boot#16230, spring-projects/spring-framework#25131. Original pull request: #445.
|
Not sure if related to this change since setting spring.data.jpa.repositories.bootstrap-mode=default
For #1 we inject an entityManager in the aspect and noticed that the session is closed. This is only happening in the test environment; starting the war and calling some rest endpoint that calls in the end the repository does have the session 'open'. Tests are annotated as follows: Update 1: Update 2: |
|
@basven Could you please open a new issue so that we can investigate? A minimal sample that reproduces the problem would be much appreciated. If you are receiving two |
|
I've run into this after updating to Spring Boot 2.3.3 from 2.2 So far I've only seen deadlocks in my tests. Deadlocks seem to have disappeared after switiching my DataJpaTests to A Thread Dump looks somewhat similar to ctmay4's: JpaMetamodelMappingContext seems to be instantiated as a dependency of |
|
@hgarus thank you for the report and sorry this is causing an issue for you. As indicated in the comment just above yours, could you please create a separate issue with a small sample that we can use to reproduce the issue? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Spring Boot 2.1 exposes a property
spring.data.jpa.repositories.bootstrap-modethat can be set todefault(default)deferredorlazy. I suggest do change the default value for property to be changed in the following scenarios:deferred– it parallelizes the bootstrap of theEntityManagerFactoryand delays repository initialization to the end of theApplicationContextinitialization. I.e. there are performance benefits to the startup but the app is the same state as with the current default once it has been started.lazyfor@SpringBootTest,@DataJpaTest– this leaves all but repositories injected into the tests initialized which gives a bit of a performance boost in both scenarios.The text was updated successfully, but these errors were encountered: