jooby icon indicating copy to clipboard operation
jooby copied to clipboard

Request cancellation not supported?

Open maxpert opened this issue 2 years ago • 3 comments

So I am using Jooby with Kotlin coroutines, and I can consistently verify that on client cancelling the request the cancellation is not propagated, example:

            get ("/") {
                val j = coroutineScope.launch {
                    suspendCancellableCoroutine { cnt ->
                        cnt.invokeOnCancellation {
                            logger.info("Request cancelled...", it)
                        }
                    }
                }

                delay(10_000)
                j.cancel("Manually cancelled")
            }

Hit the server with basic curl http://... and terminate curl before 10 seconds.

Expected:

Cancellation should have been invoked with cancellation error coming from stack.

Result:

Always hits manual cancellation which means main coroutine is never cancelled.

07:18:45.377 [handler-3] INFO  Main - Request cancelled...
java.util.concurrent.CancellationException: Manually cancelled
	at kotlinx.coroutines.ExceptionsKt.CancellationException(Exceptions.kt:22)
	at kotlinx.coroutines.JobKt__JobKt.cancel(Job.kt:608)
	at kotlinx.coroutines.JobKt.cancel(Unknown Source)
	at kotlinx.coroutines.JobKt__JobKt.cancel$default(Job.kt:608)
	at kotlinx.coroutines.JobKt.cancel$default(Unknown Source)
	at com.gateway.LauncherKt$main$1$2$1.invokeSuspend(Launcher.kt:61)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at net.openhft.affinity.AffinityThreadFactory$1.run(AffinityThreadFactory.java:60)
	at java.base/java.lang.Thread.run(Thread.java:833)

maxpert avatar Oct 08 '21 14:10 maxpert

Hi,

I'm not sure we can ensure such behavior across web servers (netty, jetty, undertow).

Also, client disconnect occurs all the time and mostly they are error we want to ignore. See: https://github.com/jooby-project/jooby/blob/2.x/jooby/src/main/java/io/jooby/Server.java#L193

jknack avatar Oct 08 '21 14:10 jknack

Also when clients close connections most of them do not really do a good job. They sometimes pseudo close a connection in mid HTTP message/frame(/2). So the server isn't even sure if the connection is really closed. Sometimes the server sends an TCP RST packet and this usually throws an exception somewhere which is one way to form a denial of service as exception throwing can be slow (that is opening connection and forcibly closing). In lots of frameworks you can catch that exception but its usually bad as jknack said. I assume curl doesn't do this.

Anyway most of the above is TCP (HTTP1/2 but not 3) layer so its probably very different across servers.

If you want to experience this just put up a HTTP server like NGINX with a public IP and port opened and watch the stream of bots hit it. Most of them don't even close the connections (which is one of the reasons why its good to have a reverse proxy in front).

agentgt avatar Oct 08 '21 16:10 agentgt

I think couple of issues being mixed here:

  • Some clients may misbehave and have these weird states, but that is a different problem to tackle. Frameworks like Netty, Undertow etc. provide you plain mechanism to let you know that underlaying channel was closed. It should be headache the transport underneath not Jooby to know when a connection was closed. Not having mechanism in first place is first limit we should get over.
  • I get the argument that some of the underlaying transport servers like Jetty might not support the notification mechanism. Jooby should simply say if underlaying frameworks supports it we will otherwise we won't. Restricting a feature that can be useful for production scenarios (specially expensive endpoints) is a downer in my opinion. Spring Webflux supports it, Micronaut supports it and there is a reason they support it.
  • Bots misusing lingering connections is a totally different landscape, and there are different ways to handle that. I have not seen frameworks putting that question up to developer, at that scale the edge proxies do all the heavy lifting. I don't see that as an argument as well against not supporting cancellations.

Again I can tell you one scenario right on top of my head that will happen in production and this feature will help servers shedding load. In dire times if server gets back logged specially in cases where event loop is accepting connections but the workers are not able to make progress, quite a few clients will end up timing out and cancelling requests in normal world. With this support missing developers have no way to cancel progress on clients who withdrew or timed-out. I've been through this in production during outages and I know how much of a life saver this was in Spring Webflux. I would insist on keeping this on table.

maxpert avatar Oct 23 '21 16:10 maxpert

I couldn't find how to do it. PR is welcome in case you want to do it.

jknack avatar May 23 '23 00:05 jknack