Discard Entity on Default Exception Handler #2084 (#2209)

Fixes #2084

akka-http-tests/src/test/scala/akka/http/scaladsl/server/DiscardEntityDefaultExceptionHandlerSpec.scala

@@ -0,0 +1,61 @@
+/*
+ * Copyright (C) 2009-2018 Lightbend Inc. <https://www.lightbend.com>
+ */
+
+package akka.http.scaladsl.server
+
+import akka.http.scaladsl.model.ContentTypes.`text/plain(UTF-8)`
+import akka.http.scaladsl.model.HttpEntity
+import akka.http.scaladsl.model.StatusCodes.InternalServerError
+import akka.stream.scaladsl.Source
+import akka.util.ByteString
+import org.scalatest.concurrent.Eventually._
+import org.scalatest.concurrent.ScalaFutures
+
+import scala.concurrent.Future
+
+class DiscardEntityDefaultExceptionHandlerSpec extends RoutingSpec with ScalaFutures {
+
+  private val route = concat(
+    path("crash") {
+      throw new RuntimeException("BOOM!")
+    }, path("crashAfterConsuming") {
+      extractRequestEntity { entity ⇒
+        val future: Future[String] = entity.dataBytes.runFold(ByteString.empty)(_ ++ _).map(_ ⇒ throw new RuntimeException("KABOOM!"))
+        complete(future)
+      }
+    }
+  )
+
+  trait Fixture {
+    @volatile
+    var streamConsumed = false
+    val thousandElements: Stream[ByteString] = Stream.continually(ByteString("foo")).take(999).append {
+      streamConsumed = true
+      Seq(ByteString("end"))
+    }
+
+  }
+
+  "Default ExceptionHandler" should {
+    "rejectEntity by default" in new Fixture {
+      streamConsumed shouldBe false
+      Get("/crash", HttpEntity(`text/plain(UTF-8)`, Source[ByteString](thousandElements))) ~> Route.seal(route) ~> check {
+        status shouldBe InternalServerError
+        eventually { // Stream will be eventually consumed, once all the stream bytes are successfully discarded
+          streamConsumed shouldBe true
+        }
+      }
+    }
+
+    "rejectEntity by default even if consumed already" in new Fixture {
+      streamConsumed shouldBe false
+      Get("/crashAfterConsuming", HttpEntity(`text/plain(UTF-8)`, Source[ByteString](thousandElements))) ~> Route.seal(route) ~> check {
+        // Stream should be consumed immediately after the request finishes
+        streamConsumed shouldBe true
+        status shouldBe InternalServerError
+      }
+    }
+  }
+
+}

akka-http-tests/src/test/scala/akka/http/scaladsl/server/TestServer.scala

@@ -49,7 +49,7 @@ object TestServer extends App {
       path("") {
         withRequestTimeout(1.milli, _ ⇒ HttpResponse(
           StatusCodes.EnhanceYourCalm,
-          entity = "Unable to serve response within time limit, please enchance your calm.")) {
+          entity = "Unable to serve response within time limit, please enhance your calm.")) {
           Thread.sleep(1000)
           complete(index)
         }

akka-http-tests/src/test/scala/akka/http/scaladsl/server/directives/TimeoutDirectivesSpec.scala

@@ -34,7 +34,7 @@ class TimeoutDirectivesSpec extends IntegrationRoutingSpec {
   "allow mapping the response" in {
     val timeoutResponse = HttpResponse(
       StatusCodes.EnhanceYourCalm,
-      entity = "Unable to serve response within time limit, please enchance your calm.")
+      entity = "Unable to serve response within time limit, please enhance your calm.")
 
     val route =
       path("timeout") {

akka-http/src/main/scala/akka/http/scaladsl/server/ExceptionHandler.scala

@@ -41,15 +41,20 @@ object ExceptionHandler {
         if (!knownToBeSealed) ExceptionHandler(knownToBeSealed = true)(this orElse default(settings)) else this
     }
 
+  /**
+   * Default [[ExceptionHandler]] that discards the request's entity by default.
+   */
   def default(settings: RoutingSettings): ExceptionHandler =
     apply(knownToBeSealed = true) {
       case IllegalRequestException(info, status) ⇒ ctx ⇒ {
         ctx.log.warning("Illegal request: '{}'. Completing with {} response.", info.summary, status)
+        ctx.request.discardEntityBytes(ctx.materializer)
         ctx.complete((status, info.format(settings.verboseErrorMessages)))
       }
       case NonFatal(e) ⇒ ctx ⇒ {
         val message = Option(e.getMessage).getOrElse(s"${e.getClass.getName} (No error message supplied)")
         ctx.log.error(e, ErrorMessageTemplate, message, InternalServerError)
+        ctx.request.discardEntityBytes(ctx.materializer)
         ctx.complete(InternalServerError)
       }
     }

docs/src/main/paradox/release-notes/10.1.x.md

@@ -1,5 +1,17 @@
 # 10.1.x Release Notes
 
+## 10.1.6
+
+10.1.6 is the seventh release in the 10.1.x series of Akka HTTP.
+
+### **Changes since 10.1.4**
+
+For a full overview you can also see the [10.1.6 milestone](https://github.com/akka/akka-http/milestone/44?closed=1):
+
+#### Fixes in akka-http-core
+ * Default exception handlers do now discard entity bytes when completing a request that ended in error ([#2084](https://github.com/akka/akka-http/issues/2084))
+
+
 ## 10.1.5
 
 10.1.5 is the sixth release in the 10.1.x series of Akka HTTP.

docs/src/main/paradox/routing-dsl/exception-handling.md

@@ -79,6 +79,13 @@ To understand the performance implications of (mis-)using exceptions,
 have a read at this excellent post by A. Shipilёv: [The Exceptional Performance of Lil' Exception](https://shipilev.net/blog/2014/exceptional-performance).
 @@@
 
+
+@@@ note
+Please note that since version `10.1.6`, the default `ExceptionHandler` will also discard the entity bytes automatically. If you want to change this behavior,
+please refer to @ref[the section above](exception-handling.md#exception-handling); however, might cause connections to stall
+if the entity is not properly rejected or cancelled on the client side.
+@@@
+
 ## Respond with headers and Exception Handler
 
 If you wrap an ExceptionHandler inside a different directive, then that directive will still apply. Example below shows

docs/src/main/paradox/routing-dsl/rejections.md

@@ -43,7 +43,7 @@ will handle *all* rejections that reach it.
 
 @@@ note
 Please note that since version `10.1.2`, the default `RejectionHandler` will also discard the entity bytes automatically. If you want to change this behavior,
-please refer to @ref[Customising rejection HTTP Responses](rejections.md#customising-rejections) if you want to change this behavior; however, might cause connections to stall 
+please refer to @ref[Customising rejection HTTP Responses](rejections.md#customising-rejections); however, might cause connections to stall 
 if the entity is not properly rejected or cancelled on the client side.
 @@@