fixup! fixup! KTOR-7139 Fux exception thrown in onCallRespond makes t…

…he client wait for response indefinitely
ktorio · Feb 4, 2025 · 23cb99e · 23cb99e
1 parent 0c1cdc8
commit 23cb99e
Show file tree

Hide file tree

Showing 10 changed files with 51 additions and 37 deletions.
diff --git a/ktor-server/ktor-server-core/common/src/io/ktor/server/engine/BaseApplicationEngine.kt b/ktor-server/ktor-server-core/common/src/io/ktor/server/engine/BaseApplicationEngine.kt
@@ -51,7 +51,7 @@ public abstract class BaseApplicationEngine(
         val pipeline = pipeline
 
         BaseApplicationResponse.setupSendPipeline(pipeline.sendPipeline)
-        BaseApplicationResponse.setupFallbackResponse(pipeline)
+        BaseApplicationResponse.setupFallbackResponse(pipeline, environment.log)
 
         monitor.subscribe(ApplicationStarting) {
             if (!info.isFirstLoading) {

diff --git a/ktor-server/ktor-server-core/common/src/io/ktor/server/engine/BaseApplicationResponse.kt b/ktor-server/ktor-server-core/common/src/io/ktor/server/engine/BaseApplicationResponse.kt
@@ -13,6 +13,7 @@ import io.ktor.server.response.*
 import io.ktor.util.*
 import io.ktor.util.cio.*
 import io.ktor.util.internal.*
+import io.ktor.util.logging.Logger
 import io.ktor.utils.io.*
 import kotlinx.coroutines.CopyableThrowable
 import kotlinx.coroutines.Dispatchers
@@ -341,14 +342,16 @@ public abstract class BaseApplicationResponse(
             }
         }
 
-        public fun setupFallbackResponse(application: EnginePipeline) {
+        public fun setupFallbackResponse(application: EnginePipeline, logger: Logger) {
             val inDevMode = application.developmentMode
             application.intercept(EnginePipeline.Before) {
                 try {
                     proceed()
                 } catch (cause: Throwable) {
                     if (call.isHandled) return@intercept
 
+                    logger.error("Unhandled server error: \"${cause.message}\"", cause)
+
                     val response = call.response as? BaseApplicationResponse
                         ?: call.attributes[EngineResponseAttributeKey]
 

diff --git a/ktor-server/ktor-server-core/common/src/io/ktor/server/engine/DefaultEnginePipeline.kt b/ktor-server/ktor-server-core/common/src/io/ktor/server/engine/DefaultEnginePipeline.kt
@@ -76,15 +76,13 @@ public suspend fun logError(call: ApplicationCall, error: Throwable) {
  *
  * [Report a problem](https://ktor.io/feedback/?fqname=io.ktor.server.engine.defaultExceptionStatusCode)
  */
-public fun defaultExceptionStatusCode(cause: Throwable): HttpStatusCode? {
-    return when (cause) {
-        is BadRequestException -> HttpStatusCode.BadRequest
-        is NotFoundException -> HttpStatusCode.NotFound
-        is UnsupportedMediaTypeException -> HttpStatusCode.UnsupportedMediaType
-        is PayloadTooLargeException -> HttpStatusCode.PayloadTooLarge
-        is TimeoutException, is TimeoutCancellationException -> HttpStatusCode.GatewayTimeout
-        else -> null
-    }
+public fun defaultExceptionStatusCode(cause: Throwable): HttpStatusCode? = when (cause) {
+    is BadRequestException -> HttpStatusCode.BadRequest
+    is NotFoundException -> HttpStatusCode.NotFound
+    is UnsupportedMediaTypeException -> HttpStatusCode.UnsupportedMediaType
+    is PayloadTooLargeException -> HttpStatusCode.PayloadTooLarge
+    is TimeoutException, is TimeoutCancellationException -> HttpStatusCode.GatewayTimeout
+    else -> null
 }
 
 private suspend fun tryRespondError(call: ApplicationCall, statusCode: HttpStatusCode) {

diff --git a/...server/ktor-server-core/common/src/io/ktor/server/engine/internal/ExceptionPageContent.kt b/...server/ktor-server-core/common/src/io/ktor/server/engine/internal/ExceptionPageContent.kt
@@ -4,6 +4,7 @@
 
 package io.ktor.server.engine.internal
 
+import io.ktor.http.HttpStatusCode
 import io.ktor.http.content.OutgoingContent
 import io.ktor.server.application.ApplicationCall
 import io.ktor.server.plugins.origin
@@ -13,6 +14,9 @@ import io.ktor.utils.io.ByteReadChannel
 
 internal class ExceptionPageContent(call: ApplicationCall, cause: Throwable) : OutgoingContent.ReadChannelContent() {
 
+    override val status: HttpStatusCode?
+        get() = HttpStatusCode.InternalServerError
+
     private val responsePage: String = buildString {
         val request = call.request
         append("<html><body><h1>Internal Server Error</h1><h2>Request Information:</h2><pre>")

diff --git a/...rver/ktor-server-plugins/ktor-server-auth/common/test/io/ktor/tests/auth/BasicAuthTest.kt b/...rver/ktor-server-plugins/ktor-server-auth/common/test/io/ktor/tests/auth/BasicAuthTest.kt
@@ -17,7 +17,10 @@ import io.ktor.server.testing.*
 import io.ktor.util.*
 import io.ktor.utils.io.charsets.*
 import io.ktor.utils.io.core.*
-import kotlin.test.*
+import kotlin.test.Test
+import kotlin.test.assertEquals
+import kotlin.test.assertNotEquals
+import kotlin.test.assertNotNull
 
 class BasicAuthTest {
     @Test
@@ -213,17 +216,15 @@ class BasicAuthTest {
             }
         }
 
-        assertFailsWith<NotImplementedError> {
-            handleRequestWithBasic("/", "u", "a")
-        }
+        assertEquals(HttpStatusCode.InternalServerError, handleRequestWithBasic("/", "u", "a").status)
     }
 
     private suspend fun ApplicationTestBuilder.handleRequestWithBasic(
         url: String,
         user: String,
         pass: String,
         charset: Charset = Charsets.ISO_8859_1
-    ) = client.get(url) {
+    ): HttpResponse = client.get(url) {
         val up = "$user:$pass"
         val encoded = up.toByteArray(charset).encodeBase64()
         header(HttpHeaders.Authorization, "Basic $encoded")

diff --git a/...ver/ktor-server-plugins/ktor-server-auth/common/test/io/ktor/tests/auth/BearerAuthTest.kt b/...ver/ktor-server-plugins/ktor-server-auth/common/test/io/ktor/tests/auth/BearerAuthTest.kt
@@ -12,7 +12,8 @@ import io.ktor.server.auth.*
 import io.ktor.server.response.*
 import io.ktor.server.routing.*
 import io.ktor.server.testing.*
-import kotlin.test.*
+import kotlin.test.Test
+import kotlin.test.assertEquals
 
 class BearerAuthTest {
 
@@ -132,11 +133,11 @@ class BearerAuthTest {
             authenticate = { throw NotImplementedError() }
         )
 
-        assertFailsWith<NotImplementedError> {
-            client.get("/") {
-                withToken("letmein")
-            }
+        val response = client.get("/") {
+            withToken("letmein")
         }
+
+        assertEquals(HttpStatusCode.InternalServerError, response.status)
     }
 
     @Test

diff --git a/...tor-server-status-pages/common/test/io/ktor/server/plugins/statuspages/StatusPagesTest.kt b/...tor-server-status-pages/common/test/io/ktor/server/plugins/statuspages/StatusPagesTest.kt
@@ -17,8 +17,10 @@ import io.ktor.server.routing.*
 import io.ktor.server.testing.*
 import io.ktor.utils.io.*
 import io.ktor.utils.io.charsets.*
-import kotlinx.coroutines.*
 import kotlinx.coroutines.CancellationException
+import kotlinx.coroutines.async
+import kotlinx.coroutines.delay
+import kotlinx.coroutines.launch
 import kotlin.test.*
 
 class StatusPagesTest {
@@ -246,9 +248,7 @@ class StatusPagesTest {
             }
         }
 
-        assertFails {
-            client.get("/")
-        }
+        assertEquals(HttpStatusCode.NotFound, client.get("/").status)
     }
 
     @Test

diff --git a/ktor-server/ktor-server-test-host/common/src/io/ktor/server/testing/TestApplicationEngine.kt b/ktor-server/ktor-server-test-host/common/src/io/ktor/server/testing/TestApplicationEngine.kt
@@ -118,7 +118,7 @@ public class TestApplicationEngine(
 
         val throwOnException = environment.config
             .propertyOrNull(CONFIG_KEY_THROW_ON_EXCEPTION)
-            ?.getString()?.toBoolean() ?: true
+            ?.getString()?.toBoolean() != false
         tryRespondError(
             defaultExceptionStatusCode(cause)
                 ?: if (throwOnException) throw cause else HttpStatusCode.InternalServerError

diff --git a/.../ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/SustainabilityTestSuite.kt b/.../ktor-server-test-suites/jvm/src/io/ktor/server/testing/suites/SustainabilityTestSuite.kt
@@ -22,6 +22,7 @@ import io.ktor.server.routing.*
 import io.ktor.server.test.base.*
 import io.ktor.server.testing.*
 import io.ktor.util.cio.*
+import io.ktor.util.pipeline.PipelinePhase
 import io.ktor.utils.io.*
 import io.ktor.utils.io.core.*
 import io.ktor.utils.io.jvm.javaio.*
@@ -795,19 +796,22 @@ abstract class SustainabilityTestSuite<TEngine : ApplicationEngine, TConfigurati
         val loggerDelegate = LoggerFactory.getLogger("ktor.test")
         val logger = object : Logger by loggerDelegate {
             override fun error(message: String?, cause: Throwable?) {
-                println(cause.toString())
                 exceptions.add(cause!!)
             }
         }
-        val phase = EnginePipeline.Before
+
         val server = createServer(log = logger) {
             routing {
                 get("/req") {
                     call.respond("SUCCESS")
                 }
             }
         }
-        (server.engine as BaseApplicationEngine).pipeline.intercept(phase) {
+
+        val phase = PipelinePhase("test")
+        val pipeline = (server.engine as BaseApplicationEngine).pipeline
+        pipeline.insertPhaseBefore(EnginePipeline.Before, phase)
+        pipeline.intercept(EnginePipeline.Before) {
             throw IllegalStateException("Failed in engine pipeline")
         }
         startServer(server)
@@ -818,7 +822,7 @@ abstract class SustainabilityTestSuite<TEngine : ApplicationEngine, TConfigurati
             }
         }) {
             assertEquals(HttpStatusCode.InternalServerError, status, "Failed in engine pipeline")
-            assertEquals(exceptions.size, 1, "Failed in phase $phase")
+            assertEquals(1, exceptions.size, "Failed in phase $phase")
             assertEquals("Failed in engine pipeline", exceptions[0].message)
             exceptions.clear()
         }

diff --git a/ktor-server/ktor-server-tests/jvm/test/io/ktor/server/http/RespondWriteTest.kt b/ktor-server/ktor-server-tests/jvm/test/io/ktor/server/http/RespondWriteTest.kt
@@ -6,11 +6,13 @@ package io.ktor.server.http
 
 import io.ktor.client.request.*
 import io.ktor.client.statement.*
+import io.ktor.http.HttpStatusCode
 import io.ktor.server.application.*
 import io.ktor.server.response.*
 import io.ktor.server.routing.*
 import io.ktor.server.testing.*
 import kotlinx.coroutines.*
+import org.apache.http.HttpStatus
 import java.util.concurrent.*
 import kotlin.test.*
 
@@ -38,9 +40,9 @@ class RespondWriteTest {
             }
         }
 
-        assertFailsWith<IllegalStateException> {
-            client.get("/")
-        }
+        val response = client.get("/")
+        assertEquals(HttpStatusCode.OK, response.status)
+        assertEquals("", response.bodyAsText())
     }
 
     @Test
@@ -69,18 +71,19 @@ class RespondWriteTest {
         routing {
             get("/") {
                 call.respondTextWriter {
+                    write("OK")
                     close() // after that point the main pipeline is going to continue since the channel is closed
                     // so we can't simply bypass an exception
                     // the workaround is to hold pipeline on channel pipe until commit rather than just close
 
-                    Thread.sleep(1000)
+                    delay(1000)
                     throw IllegalStateException("expected")
                 }
             }
         }
 
-        assertFailsWith<IllegalStateException> {
-            client.get("/")
-        }
+        val response = client.get("/")
+        assertEquals(HttpStatusCode.OK, response.status)
+        assertEquals("OK", response.bodyAsText())
     }
 }