-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Hibernate Reactive Session/EntityManager is closed #23269
Comments
/cc @DavideD, @Sanne, @gavinking |
@geoand I'm not sure whether this bug is caused by hibernate-reactive or resteasy-reactive as it's only happening when a request filter is involved, so I'm tagging you as well 😄 |
I don't really see how it could be caused by RESTEasy Reactive, but I'll try and take a look next week |
I don't see anything wrong from a RESTEasy Reactive perspective. Just for completeness sake I'll mention that the filter does run a Vert.x event-loop thread as expected. |
Thanks for checking! |
I've had a debugging session with @DavideD, we figured that the problem is caused by the fact that the So the two sessions are the same instance, which is totally OK and expected from the point of view of Hibernate Reactive and vert.x. However, the method annotated with I'm not sure why this happens but it seems to relate with the fact that @geoand / @FroMage ? It seems a bit ironical that RestEasy Reactive doesn't support a reactively implemented filter? I don't see what we can do in HR to improve on this, other than possibly be even more strict with checks and aggressively throw an exception, as I'm worried that this only gets identified as a problem under load. @gwenneg an easy workaround would be to not use |
This should not be the case... When a
Not sure what you mean here. As I explained above, the reactive return type of the filter should result in suspending request (I will make sure there isn't some kind of bug here). |
It's clearly being interleaved in this case.
I'm referring to the points described on: Lines 53 to 74 in 65f0a3d
Most likely I'm just confused, but it seems to suggest that Uni responses should be used for blocking operations; it doesn't suggest what one is supposed to do in this case while having a "fully reactive" chain. |
I'll take a closer look.
I agree it's not phrased well. I'll fix it. |
thanks @geoand ! |
👍🏼 |
I am going to stand by my position that there is nothing wrong in the filter handling in RESTEasy Reactive. I verified this by taking the reproducer and changing it in the following way: @ServerRequestFilter
public Uni<Response> filter(ContainerRequestContext requestContext) {
return sf.withSession(s -> {
requestContext.getHeaders().add("test", "true");
return s.createNativeQuery("SELECT 1")
.getSingleResult()
.replaceWith(() -> null);
}
);
} Note that inside the Then I made changed the JAX-RS method to: @GET
public Uni<List<Fruit>> get(@HeaderParam("test") String testHeader) {
if (!"true".equals(testHeader)) {
throw new IllegalStateException("RR is severily misbehaving");
}
return sf.withTransaction((s,t) -> {
return s.createNamedQuery("Fruits.findAll", Fruit.class)
.getResultList();
}
);
} Now when I hit this with a lot traffic, I do get
but nowhere do I see What's more, I did the same experiment taking HR out of the picture entirely, and just used the test HTTP header I mentioned above, through a lot more load at it and it never once failed. The Javadoc issue mentioned should be fixed in #23347 |
I don't think you can test it in this way: the HR issue is only manifesting sporadically and with high load, and it's helped by the fact that the span of events being covered is quite long: we do need to send two operations to the RDBMS, and then wait for results for these. But thanks for the idea, I will play with some variations of that. Worst case I will learn how wrong I am :) |
I'm very interested in what you find! Let me know so I can dig into this more if needed |
Complex discussion followed up: In short, I believe RestEasy Reactive should isolate the vertx context, but others aren't convinced of this; it might take some time. In that case @gwenneg you won't be able to use context-bound objects, which are implicitly relied on if you use |
Also: I've adapted the reproducer to verify our requirements without using Hibernate Reactive: It also fails only under load, but it might be a little simpler to understand why. |
The reproducer is excellent in showing what the thorny issue is. TL;DR: Hibernate Reactive expects |
Thanks! |
Is this now not required anymore, as this issue is closed? |
A bit late but that's correct, it should work as expected now |
Describe the bug
This may look similar to #22433 (which was fixed in
2.6.3.Final
) but this time the reproducer includes a resteasy-reactive request filter (seeRequestFilter
in the reproducer).Here's the exception which is thrown randomly when the app is processing concurrent requests:
Expected behavior
No response
Actual behavior
No response
How to Reproduce?
Reproducer: https://github.com/gwenneg/quarkus-hibernate-reactive-server-request-filter
Steps to reproduce the behavior:
./mvnw clean quarkus:dev
2.3.1.Final
, the exception should never be thrown. As a consequence, this looks like a regression to us but I'll wait for a confirmation on that specific point before tagging the issue as a regression.Output of
uname -a
orver
Linux glepage.remote.csb 4.18.0-348.2.1.el8_5.x86_64 #1 SMP Mon Nov 8 13:30:15 EST 2021 x86_64 x86_64 x86_64 GNU/Linux
Output of
java -version
openjdk version "11.0.13" 2021-10-19 LTS
GraalVM version (if different from Java)
No response
Quarkus version or git rev
2.6.3.Final and 2.7.0.Final
Build tool (ie. output of
mvnw --version
orgradlew --version
)No response
Additional information
No response
The text was updated successfully, but these errors were encountered: