suspendapp icon indicating copy to clipboard operation
suspendapp copied to clipboard

preWait not working since Ktor v2.3.0 by default

Open mivanilov opened this issue 1 year ago • 4 comments

Since Ktor v2.3.0 shutdown hook was added to Ktor engines e.g. CIO and Netty, making Ktor server to stop before waiting a preWait duration configured by SuspendApp.
Adding an example project to reproduce this issue issue-demo.zip

Steps to reproduce it:

  • Start app with -Dio.ktor.server.engine.ShutdownHook=true (true by default)
  • curl http://localhost/ping -v to get 200 response
  • kill -s SIGTERM `lsof -t -i:80`
  • curl http://localhost/ping -v again to get an error while it is expected to get 200 response for a period of preWait duration before engine.stop is called.

Setting -Dio.ktor.server.engine.ShutdownHook=false fixes this issue.
To save developers time troubleshooting this issue, probably SuspendApp readme/docs should mention this Ktor engine configuration bit making sure SuspendApp works as expected?
Also might be worth adding an integration test to cover this behaviour.

mivanilov avatar Feb 05 '24 19:02 mivanilov

Hey @mivanilov,

Thank you for the report!! We could potentially also manually put Dio.ktor.server.engine.ShutdownHook to false, and print a warning if the user left it to true. 🤔 Or some other kind of logging.

What woud you expect to happen? It's not clear to me from your comment, or would you prefer it to blow up?

nomisRev avatar Apr 22 '24 12:04 nomisRev

Hey @nomisRev looking at the Ktor engines and SuspendApp code as it is now, it makes sense to me set Dio.ktor.server.engine.ShutdownHook to false as ultimately SuspendApp delays engine.stop call - which is the same function that is registered with the Ktor shutdown hook e.g. for Netty https://github.com/ktorio/ktor/blob/main/ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationEngine.kt#L214 Also I would probably prefer it blow up if Dio.ktor.server.engine.ShutdownHook is set to true as in such case SuspendApp just won't work as there will be no expected delay before engine.stop call.

mivanilov avatar May 19 '24 14:05 mivanilov

Hey @mivanilov,

Thank you for letting me know your thoughts, I 100% agree. I am going to implement this in the next month or so, while we work on releasing Arrow 2.0 alongside the K2 release that'll come any week now. KotlinConf 👀

nomisRev avatar May 19 '24 16:05 nomisRev