SQLClient connection leak in getConnectionAwait(), leads to a deadlock about vertx-lang-kotlin HOT 11 CLOSED

I actually quite like structured concurrency principles coroutines offer. As cancellation is cooperative and it throws an exception, it is possible to reasonably close resources with r.use { } or try{ } finally{ }. However, bridging coroutines with Future callbacks is a bit more tricky with respect to cancellation. https://medium.com/androiddevelopers/cancellation-in-coroutines-aa6b90163629

The problem is that with current Future design, when handler is invoked and coroutine is not active anymore, we have no way to signal the completion to the calling code, so the logical idea is to close acquired resource.

suspend fun <T> awaitEvent(block: (h: Handler<T>) -> Unit): T {
  return suspendCancellableCoroutine { cont: CancellableContinuation<T> ->
       try {
        block.invoke(Handler { t ->
        if (cont.isActive)
          cont.resume(t)
        } else if(it is java.io.Closeable) {
          t.close()  // <--- close connection if coroutine is already cancelled when handler is invoked
        } 
      })
    } catch (e: Exception) {
      cont.resumeWithException(e)
    }
  }
}

When I experimented with non-cancellable coroutine in awaitEvent, the whole PoC froze, dbclient keeps the system locked. I would like cancelable coroutines better (as currently are) as it is more Kotlin-ic, IMHO. It provides server a way to recover from risk states and to cancel blocked coroutines.

Good point with the resources timeouts. Using timeout also on the top level of coroutine processing makes sure we didn't forget setting any timeouts and guarantee maximal execution time for a request (availability, scalability).

from vertx-lang-kotlin.

no, we are saying that we will not implement cancelation as this is not what we want to support, e.g supporting loom in the future will not be able to deal with cancelation

from vertx-lang-kotlin.

can you provide a reproducer project ? that would help

from vertx-lang-kotlin.

@vietj I hoped that my description was detailed enough to avoid implementing PoC as I am quite busy 😅

Here is the POC:
https://github.com/ph4r05/vertx-sqlconn-poc

• Check out readme for the usage. There is also a fix implemented.
• If you cannot reproduce it, increase number of clients from 500 to 2000 and dos-time from 10_000 to 20_000.
• Any local MySQL server works, no DB is needed for a simple ping SELECT 1. Unfortunately I don't have spare time to implement it without requiring a real MySQL database.

Code of interest:
https://github.com/ph4r05/vertx-sqlconn-poc/blob/659e211be57ffabde712fa750c5b5dd36f30ccde/src/main/kotlin/me/deadcode/vertx/WebServer.kt#L106-L112

Buggy ping
https://github.com/ph4r05/vertx-sqlconn-poc/blob/659e211be57ffabde712fa750c5b5dd36f30ccde/src/main/kotlin/me/deadcode/vertx/WebServer.kt#L129-L133

Ping with fixed race condition
https://github.com/ph4r05/vertx-sqlconn-poc/blob/659e211be57ffabde712fa750c5b5dd36f30ccde/src/main/kotlin/me/deadcode/vertx/WebServer.kt#L135-L179

from vertx-lang-kotlin.

can you elaborate on how the coroutine can get cancelled in your case ?

from vertx-lang-kotlin.

I understand the issue and it seems it is related to the fact that Vert.x futures cannot be cancelled and I am wondering how we could solve this. e.g we could a close called when the coroutine is cancelled and the future result implements an interface like Closeable.

from vertx-lang-kotlin.

Perhaps we could make Vert.x coroutines as non cancellable ?

from vertx-lang-kotlin.

can you elaborate on how the coroutine can get cancelled in your case ?

Coroutine gets usually cancelled due to timeout in withTimeout() as we want to limit execution time of some code paths, not to lock too many resources if server is overloaded (e.g., if DB server is overloaded or has opened maximum amount of connections) and to respond in timely manner.

Another reason is client disconnect during the request, e.g., in the websocket server handler, client may stop responding to pings, socket can get disconnected due to network issue, client disconnects when using mobile data. The coroutine chain triggered by the client request get cancelled. Similarly, the CoroutineScope may be cancelled from another reason (e.g., reloading TLS certificates, maintenance mode).

e.g we could a close called when the coroutine is cancelled and the future result implements an interface like Closeable.

I was also thinking along these lines. It seems like a fine workaround. It t comes from the Future, it is probably sound to close it if coroutine is not active anymore.

Perhaps we could make Vert.x coroutines as non cancellable ?

I am not sure about the impact of this. Personally, it feels a bit risky and maybe too restrictive.

from vertx-lang-kotlin.

I would actually discourage using such cancellation. The more I think about it, the more I see coroutine cancellation as something similar to killing a thread without having an opportunity to cleanly close all the resources associated with the coroutines.

Instead you should try to use timeout on the resources you are using and let them fail for you, e.g if your HTTP server uses a WebClient then set timeout on the HTTP requests. It will have a similar effect and has a more reliable handling.

from vertx-lang-kotlin.

Closing as we prefer not moving forward with this.

from vertx-lang-kotlin.

@tsegismont so if I understand correctly, you basically say that deadlock will be preserved in code and there is nothing you will do about it? :)

from vertx-lang-kotlin.