quarkusbot
commented
4 years ago /cc @gsmet, @Sanne
Open ThomasSousa96 opened 4 years ago
quarkusbot
commented
4 years ago /cc @gsmet, @Sanne
Sanne
commented
4 years ago Hi, thanks for reporting! We probably need to improve the error detection, but I wouldn't say this to be a valid usage: you can't use an EntityManager in a different thread than the one which created it as it's a managed component, which in turn manages several resources (such as the connection in this case).
from checking the ORM code, it looks like that to trigger this specific NPE we'd need a concurrent invocation on org.hibernate.resource.jdbc.internal.LogicalConnectionManagedImpl#releaseConnection - but I don't know what is triggering this. Since what I see is a race condition, that would explain why you see this intermittently.
Any suggestion @mkouba , @FroMage or @stuartwdouglas ? Could we do better, or at least detect and block using such a pattern?
Sanne
commented
4 years ago I've pushed this change to Hibernate ORM:
Wondering if Quarkus would be able to preven this in a better way.
famod
commented
4 years ago @Sanne I am actually a bit surprised that this usage scenario is not supported? I mean, the EM is not (directly) injected into the bean that is using the executor. TBH, given all the "laziness" that is usually happening during CDI injection, I also would have expected that this scenario should just work... What would be the "right" way to do it?
Sanne
commented
4 years ago I mean, the EM is not (directly) injected into the bean that is using the executor.
The problem I see in the code above is that the EntityManager is being created in one thread (the one requesting the injection) and then that one single EntityManager is passed to N threads to be used. The EntityManager is not threadsafe.
What would be the "right" way to do it?
I'm not familiar with context propagation (so perhaps there's a better way to do this) but the way I'd do this myself is to pass the EntityManagerFactory to other threads, and then explicitly open/close a new EntityManager within each "task" scope.
In particular when writing code which does explicit threading I'd tend to favour being all-in explicit with scope boundaries of the EntityManager lifecycle, its fllush and load cycles, and transaction boundaries. Alternatively, if simpicity is more important, refrain from explicit threading.
famod
commented
4 years ago Ok, I can see it now: All Bar instances are resolved outside of runAsync() and therefore each @RequestScoped Bar instance is associated to the "original" request/thread, but not to the respective ManagedExecutor thread(s).
So either each Bar instance would have to do the executor handling on its own or the lookup of each Bar instance has to be deferred to runAsync().
But let's see what the experts you've linked are saying...
Sanne
commented
4 years ago Exactly, @famod ; this is a violation of the ORM contracts. We could close the issue, but I'm not sure if "context propagation" is in some way suggesting that this should be doable.
And ideally I'd hope we could detect such cases and provide a better error message - I've included a small improvement in ORM but I don't think it's effective enough, as it will only trigger in rare conditions (and probably too late to prevent trouble).
FroMage
commented
4 years ago No, CP exists for the use-cases of propagating contexts that work on multiple threads provided that:
runAsync for example)You can exclude contexts by creating your own ManagedExecutor and specifying that you want all contexts cleared if you intend to manage your own ORM context in parallel threads.
Sanne
commented
4 years ago No,
Could you clarify what you're saying "No" to @FroMage ? You don't agree we should close this issue?
FroMage
commented
4 years ago I was replying to your question "I'm not sure if "context propagation" is in some way suggesting that this should be doable."
No, it should not be doable, so yes you can close this, though it'd be nice to be able to detect this.
Sanne
commented
4 years ago Can you think of a way to detect and prevent such a mistake? This seems very important to me, but I'm not familiar at all with the context propagation design.
FroMage
commented
4 years ago I can't think of any way to detect and prevent this, precisely because it's alright to access a transaction from multiple threads, as long as it's not "at the same time", so with proper user synchronisation. So, if you start an endpoint as transactional you have the TX in your thread, from which you can totally spawn another thread which captures it and as long as the operations are not done in parallel, it will work, and we want it to work. If we were to try to detect that a transaction is used by two threads we'd break this valid usage, so the TX granularity is too coarse.
Given that it's the ORM operations that are problematic, we could try to detect that an operation is ongoing, either by synchronizing them or keeping track of ongoing calls, but that's some work, and it will be costly even for people who don't try this fancy kind of death trap.
We could look at which thread is currently invoking the ORM operation, like we do for preventing it on the IO thread, but it would only tell us that we're in a worker thread, and the ManagedExecutor delegates to the worker thread, so we still can't distinguish that this is a potentially parallel thread.
So, sorry, I have several ideas of what would not work, but no idea as to what could work.
Sanne
commented
4 years ago ok, thanks @FroMage - I appreciate the thoughts. To clarify, I don't think it's problematic for an Hibernate Session to be "passed on" to a different thread, but there is a problem with exclusivity, or lack of safe state handoff. In particular the bug reported here is only possible if there's parallel operations happening, or if the full memory state hasn't been fully published to the new thread.
From that I'd conclude:
For the record point 1 bothers me, as I'd expect most code to fall into this cathegory - especially end user's code.
cc/ @gavinking
yrodiere
commented
1 year ago the problem isn't limited to ORM but extends to any non-threadsafe component one would use in such pattern
I changed the labels of this issue to reflect this.
Describe the bug Using hibernate and context propagation with
ManagedExecutoroccur an intermittent NullPointerException. I runs this code and sometimes it runs with success but other time it fail.Expected behavior No throw NullPointerException
Actual behavior
To Reproduce Steps to reproduce the behavior:
BarbeansConfiguration
Screenshots (If applicable, add screenshots to help explain your problem.)
Environment (please complete the following information):
uname -aorver: Linux 5.4.0-42-generic #46-Ubuntu SMP Fri Jul 10 00:24:02 UTC 2020 x86_64 x86_64 x86_64 GNU/Linuxjava -version: Java 8mvnw --versionorgradlew --version): 3.6.3Additional context (Add any other context about the problem here.)