Connection not closed when parent coroutine timeout #1436
Comments
|
in MutinySessionFactoryImpl#withSession theres no I saw that Transaction has processing for onCacellation. Is there a best practice to handle cancel event in (withSession, withTransaction)? |
Mmmh... that's a good point. I haven't thought of it. |
|
It seems to me that |
val cancellable = sessionFactory
.withStatelessSession { it.createNativeQuery<Long>("SELECT COUNT(1) FROM User").singleResultOrNull }
.subscribe()
.with { println("success : $it") }
val timer = Timer()
timer.schedule(object : TimerTask() {
override fun run() {
cancellable.cancel()
}
}, 150)
CountDownLatch(1).await()
In uni eventually() theres no process for cancel or terminate
|
|
I'm going to create an issue for Vert.x, but in the meantime, we should fix this. |
|
Sorry, I mean mutiny (not Vert.x) |
|
@cescoffier, @jponge What do you think? |
|
Looking at the issue of discussion about eventually api, the omission of terminate and cancel in eventually seems to be intended. |
private <S extends Mutiny.Closeable, T> Uni<T> withSession(
Uni<S> sessionUni,
Function<S, Uni<T>> work,
Context.Key<S> contextKey) {
AtomicBoolean connectionCloseCalled = new AtomicBoolean(false);
return sessionUni.chain(session -> Uni.createFrom().voidItem()
.invoke(() -> context.put(contextKey, session))
.chain(() -> work.apply(session))
.eventually(() -> {
if (!connectionCloseCalled.getAndSet(true)) {
return Uni.createFrom().voidItem()
.invoke(() -> context.remove(contextKey))
.chain(session::close);
} else {
return Uni.createFrom().voidItem();
}
})
.onCancellation().call(() -> {
if (!connectionCloseCalled.getAndSet(true)) {
return Uni.createFrom().voidItem()
.invoke(() -> context.remove(contextKey))
.chain(session::close);
} else {
return Uni.createFrom().voidItem();
}
})
);
}I tested it by adding the action of closing the session to onCancellation() I expected it work well. But, when the pool is full and enter the queue, the connection from the queue stuck connection pool. If cancel inside vertx context it works fine for me :) |
|
Well it might have been intended but from this discussion it doesn’t seem like the current behavior actually achieves what you would want it to do from the user point of view. |
|
That is to say: it’s supposed to be a |
|
eventually() is not called on cancellation. Note that when this callback is invoked following a cancellation, it will be called on the thread having called |
|
(The question about whether |
|
But then Clement, naively, that seems to make Am I missing something here? What is the use case for a “finally” which sometimes doesn’t get called? |
|
@gavinking I agree, and actually, I had to try because I was pretty sure it was called on cancellation too. @jponge is going to open an issue on mutiny. |
|
|
|
OK, great perfect, thanks folks 🙏 |
|
Meanwhile you can use |
|
Note that I'll backport the fix to Mutiny 1.x, I'm due a 1.9.0. |
|
@jponge |
|
|
|
@namjug-kim probably not. Mutiny does not have the notion of context (this comes from Vert.x, not mutiny which is just an API). |
|
Even
|
|
You could do something like: (not sure it compiles or even works, it's sketch) Uni.createFrom().deferred(() -> {
// Get the correct Vert.x context when the Uni is created
Context vertxContext = Vertx.getOrCreateContext();
// Create the Uni as before
return Uni.createFrom()
// (...)
.eventually(() -> {
// With the Vert.x context, dispatch the session close on what should be the correct thread even if it's a cancellation
vertxContext.runOnContext(() -> session.close());
});
})
// (...)Makes sense? |
|
it makes sense..! // if not in vertx context
Uni.createFrom().emitter(emitter -> vertxContext.runOnContext(handler -> session.close().subscribe().with(emitter::complete, emitter::fail)))Apart from Mutiny, Im not sure the cancel signal can be handled correctly. @Override
public Uni<Mutiny.Session> openSession() {
SessionCreationOptions options = options();
return uni(() -> connection(options.getTenantIdentifier()))
.chain(reactiveConnection -> create(reactiveConnection,
() -> new ReactiveSessionImpl(delegate, options, reactiveConnection)))
.map(s -> new MutinySessionImpl(s, this));
}If the |
Yeah, I don't think this is a problem, or at least, IIRC, it's a problem we already know how to solve, and already have code for, isn't that right @DavideD? (FTR, you would have to solve the exact same problem, in the exact same way, if you did it via |
Yes, looks like a threading issue where you need to get the proper Vert.x context for the code being run. The deferred |
Fixes #1181 See hibernate/hibernate-reactive#1436 (cherry picked from commit daa580e)
I think so. Is this what you mean? |
|
I made a test to verify the section that needs to be cleanup Connection. To complete the task, need to change it to use the Context of Hibernate-reactive rather than VertxContext, but can you please confirm that it is the correct approach? |
|
I think it would be better if you could first create a test case. You can check one of the many tests we have in the test folder for inspiration. Check the one extending |
|
@DavideD I didn't add |
|
Thanks @namjug-kim, |
I wanted to give timeout when I used the connection through MutinySessionFactory.
This is a reproduction code for reproducing a problem situation. In fact, Netty-based API servers are used with timeout, and Connection leaks occur.
In this situation, pool hang forever.
expected action is to close connection as shown in the log below.
However, the actual operation did not close Connection.
The text was updated successfully, but these errors were encountered: