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

Datasource connection deadlock #3173

Closed
lbsoft-lwsoft opened this issue Aug 21, 2023 · 19 comments · Fixed by #3203
Closed

Datasource connection deadlock #3173

lbsoft-lwsoft opened this issue Aug 21, 2023 · 19 comments · Fixed by #3203
Assignees
Labels
Milestone

Comments

@lbsoft-lwsoft
Copy link

Expected behavior

It is hoped that the new query caused by the previous query process should use the same jdbc connection.

Actual behavior

When the previous jdbc connection is not closed, hashcode triggers lazy loading and re-acquires a new jdbc connection, which may cause mutual exclusion of connections and deadlock in concurrent situations. For example, the maximum available connection pool is 2, when two threads trigger lazy loading at the same time, because the previous two connections are not released and get new connections through the connection pool, the connection pool finds that there is no available connection and suspends the thread to wait for other threads to release the connection, and both threads are stuck in getting new connections, resulting in deadlock.

Steps to reproduce

java.lang.RuntimeException
	at test.system.Ebean$TConnection.invoke(Ebean.java:369)
	at jdk.proxy2/jdk.proxy2.$Proxy161.getAutoCommit(Unknown Source)
	at io.ebeaninternal.server.transaction.JdbcTransaction.checkAutoCommit(JdbcTransaction.java:282)
	at io.ebeaninternal.server.transaction.JdbcTransaction.<init>(JdbcTransaction.java:223)
	at io.ebeaninternal.server.transaction.TransactionManager.createTransaction(TransactionManager.java:397)
	at io.ebeaninternal.server.transaction.TransactionFactoryBasic.create(TransactionFactoryBasic.java:55)
	at io.ebeaninternal.server.transaction.TransactionFactoryBasic.createReadOnlyTransaction(TransactionFactoryBasic.java:29)
	at io.ebeaninternal.server.transaction.TransactionManager.createReadOnlyTransaction(TransactionManager.java:390)
	at io.ebeaninternal.server.core.DefaultServer.createReadOnlyTransaction(DefaultServer.java:2231)
	at io.ebeaninternal.server.core.OrmQueryRequest.initTransIfRequired(OrmQueryRequest.java:278)
	at io.ebeaninternal.server.core.DefaultServer.findList(DefaultServer.java:1535)
	at io.ebeaninternal.server.core.DefaultServer.findList(DefaultServer.java:1515)
	at io.ebeaninternal.server.core.DefaultBeanLoader.executeQuery(DefaultBeanLoader.java:180)
	at io.ebeaninternal.server.core.DefaultBeanLoader.loadBean(DefaultBeanLoader.java:163)
	at io.ebeaninternal.server.core.DefaultServer.loadBean(DefaultServer.java:561)
	at io.ebeaninternal.server.loadcontext.DLoadBeanContext$LoadBuffer.loadBean(DLoadBeanContext.java:208)
	at io.ebean.bean.EntityBeanIntercept.loadBeanInternal(EntityBeanIntercept.java:852)
	at io.ebean.bean.EntityBeanIntercept.loadBean(EntityBeanIntercept.java:829)
	at io.ebean.bean.EntityBeanIntercept.preGetter(EntityBeanIntercept.java:923)
	at test.domain.entity.Root._ebean_get_reference(Root.java:6)
	at test.domain.entity.Root.hashCode(Root.java:657)
	at java.base/java.util.HashMap.hash(HashMap.java:338)
	at java.base/java.util.HashMap.put(HashMap.java:610)
	at java.base/java.util.HashSet.add(HashSet.java:221)
	at io.ebean.common.BeanSet.internalAdd(BeanSet.java:84)
	at io.ebean.common.BeanSet.internalAddWithCheck(BeanSet.java:74)
	at io.ebeaninternal.server.deploy.BaseCollectionHelp.add(BaseCollectionHelp.java:39)
	at io.ebeaninternal.server.deploy.BeanSetHelp.add(BeanSetHelp.java:19)
	at io.ebeaninternal.server.deploy.BeanPropertyAssocMany.addBeanToCollectionWithCreate(BeanPropertyAssocMany.java:271)
	at io.ebeaninternal.server.query.CQuery.setLazyLoadedChildBean(CQuery.java:417)
	at io.ebeaninternal.server.query.SqlTreeNodeBean$Load.complete(SqlTreeNodeBean.java:421)
	at io.ebeaninternal.server.query.SqlTreeNodeBean$Load.perform(SqlTreeNodeBean.java:447)
	at io.ebeaninternal.server.query.SqlTreeNodeBean.load(SqlTreeNodeBean.java:464)
	at io.ebeaninternal.server.query.SqlTreeNodeRoot.load(SqlTreeNodeRoot.java:45)
	at io.ebeaninternal.server.query.CQuery.readNextBean(CQuery.java:447)
	at io.ebeaninternal.server.query.CQuery.hasNext(CQuery.java:531)
	at io.ebeaninternal.server.query.CQuery.readCollection(CQuery.java:564)
	at io.ebeaninternal.server.query.CQueryEngine.findMany(CQueryEngine.java:379)
	at io.ebeaninternal.server.query.DefaultOrmQueryEngine.findMany(DefaultOrmQueryEngine.java:131)
	at io.ebeaninternal.server.core.OrmQueryRequest.findList(OrmQueryRequest.java:470)
	at io.ebeaninternal.server.core.DefaultServer.findList(DefaultServer.java:1536)
	at io.ebeaninternal.server.core.DefaultServer.findList(DefaultServer.java:1515)
	at io.ebeaninternal.server.core.DefaultBeanLoader.executeQuery(DefaultBeanLoader.java:180)
	at io.ebeaninternal.server.core.DefaultBeanLoader.loadMany(DefaultBeanLoader.java:45)
	at io.ebeaninternal.server.core.DefaultServer.loadMany(DefaultServer.java:546)
	at io.ebeaninternal.server.loadcontext.DLoadManyContext$LoadBuffer.loadMany(DLoadManyContext.java:228)
	at io.ebean.common.AbstractBeanCollection.lazyLoadCollection(AbstractBeanCollection.java:101)
	at io.ebean.common.BeanSet.init(BeanSet.java:137)
	at io.ebean.common.BeanSet.iterator(BeanSet.java:283)
	at java.base/java.util.Spliterators$IteratorSpliterator.estimateSize(Spliterators.java:1865)
	at java.base/java.util.Spliterator.getExactSizeIfKnown(Spliterator.java:414)
	at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:508)
	at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499)
	at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:921)
	at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:682)
	at test.domain.entity.Student.getCuDetail(Student.java:634)
	at test.domain.entity.Student.getCuDetail(Student.java:627)
	at test.dto.response.StudentRespDTO.<init>(StudentRespDTO.java:79)
	at test.repository.StudentUnavailabilityTimeRepositoryTest.lambda$test$1(StudentUnavailabilityTimeRepositoryTest.java:145)
	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539)
	at java.base/java.util.concurrent.FutureTask.run$$$capture(FutureTask.java:264)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at java.base/java.lang.Thread.run(Thread.java:833)
@lbsoft-lwsoft
Copy link
Author

This is the log of ebean getting jdbc connection that I debugged, uuid is the id of the connection, you can see from the log that when the connection 89dd1f90-72cf-47d8-93a6-990e1ec48bc0 is not closed, it gets another connection 827c9140-2345-4d3e-8220-f0c5bf1bba73, this problem causes deadlock when getting connection in high concurrency and the connection pool limits the maximum number.
img_v2_aa4332e9-0013-4d05-a5de-55219e03105g

@lbsoft-lwsoft
Copy link
Author

ebean version 12.8.3

@rob-bygrave
Copy link
Contributor

hashcode triggers lazy loading and re-acquires a new jdbc connection

So why do you want hashCode() to trigger lazy loading? Surely you don't want that to happen?

1. Change from using Set to using List for our entity collections.

The recommendation with Ebean is to use List over Set. This is unlike Hibernate which promotes the use of Set and gives Set better behaviour than List).

With Ebean we promote the use of List and in fact in version 15.x we might even enforce use of List (and even remove support for Set).

2. Remove the hashCode() and equals() implementation that is on test.domain.entity.Root.

You could for example remove hashCode() and equals() implementation altogether. Ebean does not require an implementation of these methods. They are difficult to get right and we certainly do not want implementations that invoke lazy loading here (or in a toString() implementation either).

So unless you really know what you are doing remove the hashCode() and equals() implementation that is on test.domain.entity.Root.

@lbsoft-lwsoft
Copy link
Author

But overriding hashcode is a very common requirement in Entity, for example, in this case, we need to use a Set collection to deduplicate a batch of objects externally, why can’t we reuse the connection through the context?

    
    @Column(name = "reference")
    private final String reference;

    @Override
    public int hashCode() {
        final int prime = 31;
        int result = 1;
        result = prime * result + ((reference == null) ? 0 : reference.hashCode());
        return result;
    }

@rbygrave
Copy link
Member

This hashCode() implementation above is BROKEN in that it can change its value. This means that you will be able to observe cases like for example something that is in the Set will not actually be found [as its hashCode changes and now the search looks in a different bucket].

The "Effective Java" recommendation is that hashCode() can not return different values.

Set collection to deduplicate a batch of objects externally

Most use cases are better supported by findMap(). In this case perhaps the map key is reference.

It isn't clear to me yet what you are trying to achieve but this hashCode() implementation is going to be a problem for you.

we reuse the connection through the context?

Not sure. Can't really see enough of the code yet to follow what is being done here.

@lbsoft-lwsoft
Copy link
Author

Why does hashcode change when reference is defined as immutable?

@rbygrave
Copy link
Member

we reuse the connection through the context?

I believe there is no explicit transaction created. Ebean is implicitly creating a transaction as needed to run the query.

So in OrmQueryRequest.initTransIfRequired() it will createReadOnlyTransaction() and when it does this ... this transaction is not registered with a ThreadLocal / not registered with the "context".

So I think this is why the connection is not being re-used when later on the hashCode invokes lazy loading. And then that leads to obtaining another connection from the dataSource pool etc.

So workarounds, assuming you want to keep the hashCode implementation could be:

  • Avoid lazy loading by tuning the query to include the reference
  • Or explicitly creating a transaction. This would then wrap both the query and the lazy loading that is being invoked.

@rbygrave
Copy link
Member

Why does hashcode change when reference is defined as immutable?

So yes, this code indeed looks to be ok because its a final field - my bad, I missed that.

So yes the value for reference should not mutate so the hashCode() should always return the same value.

@rbygrave
Copy link
Member

we reuse the connection through the context?

Looking at the code, it does like a bug here in that ...

In OrmQueryRequest we have initTransIfRequired() and endTransIfRequired().

Looking at initTransIfRequired():
We can see the difference in treatment between an "update query" [which uses beginTransaction() and puts the transaction in scope] and a "read only query" [which uses createReadOnlyTransaction() and DOES NOT put the transaction in scope ].

Looking at endTransIfRequired():
We see the server.clearServerTransaction(); is only used with the "update queries".

So looking at this as a bug, we'd look at a change such that the "read query" is ALSO put into transaction scope in initTransIfRequired() and also cleared from transaction scope at the end via endTransIfRequired().

With that change in place, then any lazy loading invoked internally while executing a "read only" query with implicit transactions would reuse the transaction/java.sql.Connection [and hence not get into requesting another connection from the pool].

@lbsoft-lwsoft
Copy link
Author

我们通过上下文重用连接?

我相信没有创建显式事务。Ebean 根据需要隐式创建事务以运行查询。

所以它会以及当它这样做时...**此事务未**在 ThreadLocal 中注册/未在“上下文”中注册。OrmQueryRequest.initTransIfRequired()``createReadOnlyTransaction()

所以我认为这就是为什么稍后在 hashCode 调用延迟加载时没有重用连接的原因。然后导致从数据源池等获取另一个连接。

因此,假设您要保留哈希代码实现,解决方法可能是:

  • 通过调整查询以包含reference
  • 或者显式创建事务。然后,这将包装查询和正在调用的延迟加载。

Yes, I solved the problem by explicitly creating a transaction, but I only solved this one. I wonder if I can solve this problem directly at the bottom layer, instead of implicitly discovering this situation and then explicitly adding a transaction.

@rbygrave
Copy link
Member

I think we posted at the same time there.

solve this problem directly at the bottom layer

Yes, it looks like we can fix it in OrmQueryRequest in initTransIfRequired() and endTransIfRequired() for the read-only case.

@lbsoft-lwsoft
Copy link
Author

lbsoft-lwsoft commented Aug 21, 2023

solve this problem directly at the bottom layer

Will this problem be solved at the bottom layer? If so, is there an approximate time?

@lbsoft-lwsoft
Copy link
Author

Hi Rob, I'm wondering if this issue will be resolved anytime soon? Our systems heavily depends on it and if not, it will take us a lot of efforts to put workarounds in place almost everywhere. Thanks a lot.

@rob-bygrave
Copy link
Contributor

First step is a failing test. Are you able to create a test case perhaps based on example-minimal ? https://github.com/ebean-orm-examples/example-minimal

@lbsoft-lwsoft
Copy link
Author

lbsoft-lwsoft commented Aug 28, 2023

First step is a failing test. Are you able to create a test case perhaps based on example-minimal ? https://github.com/ebean-orm-examples/example-minimal

I have completed the test code but I don’t have permission to push it up. What should I do?

remote: Permission to ebean-orm-examples/example-minimal.git denied to lbsoft-lwsoft.
fatal: unable to access 'https://github.com/ebean-orm-examples/example-minimal.git/': The requested URL returned error: 403

@rbygrave
Copy link
Member

I have completed the test code but I don’t have permission to push it up. What should I do?

Did you "create a fork"?

You won't have permission to push to the repo directly but if you have your own fork you should be able to push to that.

@lbsoft-lwsoft
Copy link
Author

lbsoft-lwsoft commented Aug 28, 2023

I have completed the test code but I don’t have permission to push it up. What should I do?

Did you "create a fork"?

You won't have permission to push to the repo directly but if you have your own fork you should be able to push to that.

Thank you, I have submitted the test code to this address: https://github.com/lbsoft-lwsoft/example-minimal/blob/test-case-3173/src/test/java/org/example/domain/ConnectionLeakTest.java

rbygrave added a commit that referenced this issue Aug 29, 2023
- The error reported was that a DataSource connection pool maxed out
- The symptom was that there was an additional lazy loading queries invoked via the hashCode/equals implementation of ManyToMany Set.
- The fix is for BeanSet init() to lazy load with onlyIds = false, that avoids the extra lazy loading query from being executed
rbygrave added a commit that referenced this issue Aug 29, 2023
#3173 - BeanSet lazy loading / Too many DataSource connections used
@rbygrave rbygrave self-assigned this Aug 29, 2023
@rbygrave rbygrave added the bug label Aug 29, 2023
@rbygrave rbygrave added this to the 13.22.0 milestone Aug 29, 2023
rbygrave added a commit that referenced this issue Aug 29, 2023
rbygrave added a commit that referenced this issue Aug 29, 2023
This shows the extra lazy loading that is occurring when clear() is
called on the [Bean]Set DUE to the hashcode/equals implementation that
is present on the entity bean that is in the Set.
rbygrave added a commit that referenced this issue Aug 29, 2023
Follow up #3173 - Add test for BeanSet clear() when hashCode/equals used
rbygrave added a commit that referenced this issue Aug 29, 2023
To avoid the 1+N lazy loading queries that are invoked due to the clear()
when the entity bean in the Set has a hashCode/equals implementation
rbygrave added a commit that referenced this issue Aug 29, 2023
Follow up #3173 - Change BeanSet clear() to lazy load ALL properties
@lbsoft-lwsoft
Copy link
Author

lbsoft-lwsoft commented Sep 4, 2023

Hi Rob, Can this fix be updated to 12.8, we are using 12.8.3 and can’t update to 13 for now? thanks

rbygrave added a commit that referenced this issue Sep 7, 2023
- BeanSet init() and initClear() load with onlyIds false because the expectation is that with BeanSet the equals/hashCode implementation can use a property
- BeanMap lazy loading, include the mapKey if defined in the lazy loading query
rbygrave added a commit that referenced this issue Sep 7, 2023
…t-loading

[12x] Backport of fix for #3173 BeanSet init(), initClear() and BeanMap
@rbygrave
Copy link
Member

rbygrave commented Sep 7, 2023

ebean version 12.16.2 has been released with the back ported patch #3218

Note: The branch maintain-v12 is the maintenance branch for 12.x releases going forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants