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

Support cancelling Kotlin coroutine handlers on connection closing #41705

Closed
calebkiage opened this issue Jul 5, 2024 · 8 comments · Fixed by #41710
Closed

Support cancelling Kotlin coroutine handlers on connection closing #41705

calebkiage opened this issue Jul 5, 2024 · 8 comments · Fixed by #41710
Labels
area/kotlin kind/enhancement New feature or request
Milestone

Comments

@calebkiage
Copy link

calebkiage commented Jul 5, 2024

I just started testing out Quarkus with Kotlin suspend handlers and it seems to work well. However, I noticed that these handlers don't support cancellation on request connection closing. I found a workaround, but it seems very cumbersome to add this to every request and would like to find out if there's a better way.

In the sample code below, endpoint /long/a cancels the delay by throwing the CancellationException when the connection is closed, but /long/b does not.

@Path("/long")
class SampleResource(private val log: Logger) {
    @GET
    @Path("a")
    @Produces(MediaType.TEXT_PLAIN)
    suspend fun longRunning(rc: RoutingContext): String = coroutineScope {
        val requestJob = Job(coroutineContext[Job])
        // Listen for the connection close event
        rc.request().connection().closeHandler {
            requestJob.cancel(CancellationException("Client connection closed"))
        }
        // Launch a coroutine within this job
        async(requestJob) {
            delay(10000)
            log.info("/long/a responding")
            "Completed request"
        }.await()
    }

    @GET
    @Path("b")
    @Produces(MediaType.TEXT_PLAIN)
    suspend fun hello(): String = coroutineScope {
        delay(10000)
        log.info("/long/b responding")
        "Completed request"
    }
}
2024-07-04 17:30:07,579 ERROR [io.qua.ver.htt.run.QuarkusErrorHandler] (vert.x-eventloop-thread-1) HTTP Request to /long/a failed, error id: 1e9c93da-0f36-484d-8557-240fba83db7f-3: java.util.concurrent.CancellationException: Client connection closed
	at ...

2024-07-04 17:30:22,693 INFO  [org.acm.SampleResource] (vert.x-eventloop-thread-0) /long/b responding

Originally posted by @calebkiage in #41696

Sample project

Copy link

quarkus-bot bot commented Jul 5, 2024

/cc @geoand (kotlin)

@geoand
Copy link
Contributor

geoand commented Jul 5, 2024

Thanks!

Mind attaching the project you used to test this?

@calebkiage
Copy link
Author

@geoand, I've added a link to a sample project.

@geoand
Copy link
Contributor

geoand commented Jul 5, 2024

🙏🏼

geoand added a commit to geoand/quarkus that referenced this issue Jul 5, 2024
When the server part of Quarkus REST is not included, prior to
this change, user's @Provider classes were not registered
for reflection

Relates to: quarkusio#41705
geoand added a commit to geoand/quarkus that referenced this issue Jul 5, 2024
geoand added a commit to geoand/quarkus that referenced this issue Jul 5, 2024
geoand added a commit to geoand/quarkus that referenced this issue Jul 5, 2024
@geoand
Copy link
Contributor

geoand commented Jul 5, 2024

Have a look at #41710 - it resolves your issue, but it does raise a question for us :)

@mschorsch
Copy link
Contributor

Nice @geoand 👍

@geoand
Copy link
Contributor

geoand commented Jul 7, 2024

🙏

@calebkiage
Copy link
Author

@geoand, I've tested version 3.13.0.CR1 and cancellation works for the server. I've however noticed that the client doesn't reset the connection.

When using the vert.x client manually, I can trigger cancellation by resetting the connection and it works fine:

withTimeoutOrNull(delayLong) {
    suspendCancellableCoroutine { cont->
        val options = HttpClientOptions().setDefaultHost("localhost").setDefaultPort(9000)
        val client = ctx.vertx().createHttpClient(options)
        client.request(io.vertx.core.http.HttpMethod.GET, "/long/a").onComplete { conn->
            if (conn.succeeded()) {
                cont.invokeOnCancellation {
                    // Trigger a connection reset on cancellation
                    conn.result().reset()
                }
                val request = conn.result()
                request.send().onComplete { resp->
                    if (resp.succeeded()) {
                        val body = resp.result().body()
                        body.onComplete { bodyState->
                            if (bodyState.succeeded()) {
                                cont.resume(bodyState.result().toString(Charsets.UTF_8))
                            } else {
                                cont.resumeWithException(bodyState.cause())
                            }
                        }
                    } else {
                        cont.resumeWithException(resp.cause())
                    }
                }
            } else {
                cont.resumeWithException(conn.cause())
            }
        }
    }
}

I think the generated clients should also call reset on cancellation. What do you think?

holly-cummins pushed a commit to holly-cummins/quarkus that referenced this issue Jul 31, 2024
holly-cummins pushed a commit to holly-cummins/quarkus that referenced this issue Jul 31, 2024
holly-cummins pushed a commit to holly-cummins/quarkus that referenced this issue Jul 31, 2024
danielsoro pushed a commit to danielsoro/quarkus that referenced this issue Sep 20, 2024
danielsoro pushed a commit to danielsoro/quarkus that referenced this issue Sep 20, 2024
danielsoro pushed a commit to danielsoro/quarkus that referenced this issue Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kotlin kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants