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

KTOR-7139 Fix exception thrown in onCallRespond makes the client wait… #4610

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion ktor-io/common/src/io/ktor/utils/io/core/Closeable.kt
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,6 @@ import kotlin.use as stdlibUse

public expect interface Closeable : AutoCloseable

@Deprecated("Use stdlib implementation instead. Remove import of this function", ReplaceWith("stdlibUse(block)"))
@Suppress("DeprecatedCallableAddReplaceWith")
@Deprecated("Use stdlib implementation instead. Remove import of this function")
public inline fun <T : Closeable?, R> T.use(block: (T) -> R): R = stdlibUse(block)
1 change: 1 addition & 0 deletions ktor-server/ktor-server-core/api/ktor-server-core.api
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,7 @@ public final class io/ktor/server/engine/BaseApplicationResponse$BodyLengthIsToo

public final class io/ktor/server/engine/BaseApplicationResponse$Companion {
public final fun getEngineResponseAttributeKey ()Lio/ktor/util/AttributeKey;
public final fun setupFallbackResponse (Lio/ktor/server/engine/EnginePipeline;)V
public final fun setupSendPipeline (Lio/ktor/server/response/ApplicationSendPipeline;)V
}

Expand Down
1 change: 1 addition & 0 deletions ktor-server/ktor-server-core/api/ktor-server-core.klib.api
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,7 @@ abstract class io.ktor.server.engine/BaseApplicationResponse : io.ktor.server.re
final val EngineResponseAttributeKey // io.ktor.server.engine/BaseApplicationResponse.Companion.EngineResponseAttributeKey|{}EngineResponseAttributeKey[0]
final fun <get-EngineResponseAttributeKey>(): io.ktor.util/AttributeKey<io.ktor.server.engine/BaseApplicationResponse> // io.ktor.server.engine/BaseApplicationResponse.Companion.EngineResponseAttributeKey.<get-EngineResponseAttributeKey>|<get-EngineResponseAttributeKey>(){}[0]

final fun setupFallbackResponse(io.ktor.server.engine/EnginePipeline) // io.ktor.server.engine/BaseApplicationResponse.Companion.setupFallbackResponse|setupFallbackResponse(io.ktor.server.engine.EnginePipeline){}[0]
final fun setupSendPipeline(io.ktor.server.response/ApplicationSendPipeline) // io.ktor.server.engine/BaseApplicationResponse.Companion.setupSendPipeline|setupSendPipeline(io.ktor.server.response.ApplicationSendPipeline){}[0]
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ public abstract class BaseApplicationEngine(
val pipeline = pipeline

BaseApplicationResponse.setupSendPipeline(pipeline.sendPipeline)
BaseApplicationResponse.setupFallbackResponse(pipeline, environment.log)

monitor.subscribe(ApplicationStarting) {
if (!info.isFirstLoading) {
Expand All @@ -63,6 +64,7 @@ public abstract class BaseApplicationEngine(
it.installDefaultInterceptors()
it.installDefaultTransformationChecker()
}

monitor.subscribe(ApplicationStarted) {
val finishedAt = getTimeMillis()
val elapsedTimeInSeconds = (finishedAt - info.initializedStartAt) / 1_000.0
Expand Down Expand Up @@ -115,7 +117,7 @@ private fun Application.installDefaultTransformationChecker() {
intercept(ApplicationCallPipeline.Plugins) {
try {
proceed()
} catch (e: CannotTransformContentToTypeException) {
} catch (_: CannotTransformContentToTypeException) {
call.respond(HttpStatusCode.UnsupportedMediaType)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,16 @@ 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.*
import kotlinx.coroutines.CopyableThrowable
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.withContext

private val ERROR_CONTENT = object : OutgoingContent.NoContent() {
override val status: HttpStatusCode = HttpStatusCode.InternalServerError
}

public abstract class BaseApplicationResponse(
final override val call: PipelineCall
Expand Down Expand Up @@ -80,10 +88,12 @@ public abstract class BaseApplicationResponse(
// TODO: What should we do if TransferEncoding was set and length is present?
headers.append(HttpHeaders.ContentLength, contentLength.toStringFast(), safeOnly = false)
}

!transferEncodingSet -> {
when (content) {
is OutgoingContent.ProtocolUpgrade -> {
}

is OutgoingContent.NoContent -> headers.append(HttpHeaders.ContentLength, "0", safeOnly = false)
else -> headers.append(HttpHeaders.TransferEncoding, "chunked", safeOnly = false)
}
Expand Down Expand Up @@ -331,5 +341,31 @@ public abstract class BaseApplicationResponse(
response.respondOutgoingContent(body)
}
}

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]

val content = if (inDevMode) {
ExceptionPageContent(call, cause)
} else {
object : OutgoingContent.NoContent() {
override val status: HttpStatusCode = HttpStatusCode.InternalServerError
}
}

response.respondOutgoingContent(content)
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/*
* Copyright 2014-2025 JetBrains s.r.o and contributors. Use of this source code is governed by the Apache 2.0 license.
*/

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
import io.ktor.server.request.httpMethod
import io.ktor.server.request.path
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>")
append("Method: ${request.httpMethod}\n")
append("Path: ${request.path()}\n")
append("Parameters: ${request.rawQueryParameters}\n")
append("From origin: ${request.origin}\n")
append("</pre><h2>Stack Trace:</h2><pre>")

val stackTrace = cause.stackTraceToString().lines()
stackTrace.forEach { element ->
append("<span style=\"color:blue;\">$element</span><br>")
}
var currentCause = cause.cause
while (currentCause != null) {
append("<br>Caused by:<br>")
val causeStack = currentCause.stackTraceToString().lines()
causeStack.forEach { element ->
append("<span style=\"color:green;\">$element</span><br>")
}
currentCause = currentCause.cause
}
append("</pre></body></html>")
}

override fun readFrom(): ByteReadChannel = ByteReadChannel(responsePage)
}
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ internal class NettyApplicationCallHandler(
else ->
try {
enginePipeline.execute(call)
} catch (error: Exception) {
} catch (error: Throwable) {
handleFailure(call, error)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -246,9 +248,7 @@ class StatusPagesTest {
}
}

assertFails {
client.get("/")
}
assertEquals(HttpStatusCode.NotFound, client.get("/").status)
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -334,8 +334,6 @@ actual abstract class EngineTestBase<

followRedirects = false
expectSuccess = false

install(HttpRequestRetry)
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading