[master]: Stop Netty server reloading configuration on each request

documentation/manual/releases/release27/migration27/Migration27.md

@@ -92,3 +92,11 @@ Like announced in the [[Play 2.6 Migration Guide|Migration26#Java-Form-Changes]]
 ### `Guava` version updated to 24.0-jre
 
 Play 2.6.x provided 23.0 version of Guava library. Now it is updated to last actual version, 24.1-jre. Lots of changes were made in library, you can see the full changelog [here](https://github.com/google/guava/releases).
+
+### Internal changes
+
+Many changes have been made to Play's internal APIs. These APIs are used internally and don't follow a normal deprecation process. Changes may be mentioned below to help those who integrate directly with Play internal APIs.
+
+#### `Server.getHandlerFor` has moved to `Server#getHandlerFor`
+
+The `getHandlerFor` method on the `Server` trait was used internally by the Play server code when routing requests. It has been removed and replaced with a method of the same name on the `Server` object.

framework/project/BuildSettings.scala

@@ -467,7 +467,12 @@ object BuildSettings {
       ProblemFilters.exclude[MissingClassProblem]("play.api.cache.CacheApi"),
       ProblemFilters.exclude[MissingTypesProblem]("play.api.cache.DefaultSyncCacheApi"),
       ProblemFilters.exclude[DirectMissingMethodProblem]("play.api.cache.DefaultSyncCacheApi.getOrElse"),
-      ProblemFilters.exclude[DirectMissingMethodProblem]("play.api.cache.DefaultSyncCacheApi.getOrElse$default$2")
+      ProblemFilters.exclude[DirectMissingMethodProblem]("play.api.cache.DefaultSyncCacheApi.getOrElse$default$2"),
+
+      // Remove Server trait's deprecated getHandler method
+      ProblemFilters.exclude[DirectMissingMethodProblem]("play.core.server.Server.getHandlerFor"),
+      ProblemFilters.exclude[DirectMissingMethodProblem]("play.core.server.NettyServer.getHandlerFor"),
+      ProblemFilters.exclude[DirectMissingMethodProblem]("play.core.server.AkkaHttpServer.getHandlerFor")
     ),
     unmanagedSourceDirectories in Compile += {
       (sourceDirectory in Compile).value / s"scala-${scalaBinaryVersion.value}"

framework/src/play-akka-http-server/src/main/scala/play/core/server/AkkaHttpServer.scala

@@ -25,7 +25,7 @@ import play.api.libs.streams.Accumulator
 import play.api.mvc._
 import play.api.routing.Router
 import play.core.server.akkahttp.{ AkkaModelConversion, HttpRequestDecoder }
-import play.core.server.common.{ ReloadCache, ServerResultUtils }
+import play.core.server.common.{ ReloadCache, ServerDebugInfo, ServerResultUtils }
 import play.core.server.ssl.ServerSSLEngine
 import play.core.ApplicationProvider
 import play.server.SSLEngineProvider
@@ -169,7 +169,8 @@ class AkkaHttpServer(
    */
   private case class ReloadCacheValues(
       resultUtils: ServerResultUtils,
-      modelConversion: AkkaModelConversion
+      modelConversion: AkkaModelConversion,
+      serverDebugInfo: Option[ServerDebugInfo]
   )
 
   /**
@@ -186,7 +187,8 @@ class AkkaHttpServer(
         illegalResponseHeaderValue)
       ReloadCacheValues(
         resultUtils = serverResultUtils,
-        modelConversion = modelConversion
+        modelConversion = modelConversion,
+        serverDebugInfo = reloadDebugInfo(tryApp, provider)
       )
     }
   }
@@ -197,18 +199,24 @@ class AkkaHttpServer(
     reloadCache.cachedFrom(tryApp).modelConversion
 
   private def handleRequest(request: HttpRequest, secure: Boolean): Future[HttpResponse] = {
-    val remoteAddress: InetSocketAddress = remoteAddressOfRequest(request)
     val decodedRequest = HttpRequestDecoder.decodeRequest(request)
-    val requestId = requestIDs.incrementAndGet()
     val tryApp = applicationProvider.get
-    val (convertedRequestHeader, requestBodySource) = modelConversion(tryApp).convertRequest(
-      requestId = requestId,
-      remoteAddress = remoteAddress,
-      secureProtocol = secure,
-      request = decodedRequest)
-    val (taggedRequestHeader, handler, newTryApp) = getHandler(convertedRequestHeader, tryApp)
+    val (convertedRequestHeader, requestBodySource): (RequestHeader, Either[ByteString, Source[ByteString, Any]]) = {
+      val remoteAddress: InetSocketAddress = remoteAddressOfRequest(request)
+      val requestId: Long = requestIDs.incrementAndGet()
+      modelConversion(tryApp).convertRequest(
+        requestId = requestId,
+        remoteAddress = remoteAddress,
+        secureProtocol = secure,
+        request = decodedRequest)
+    }
+    val debugInfoRequestHeader: RequestHeader = {
+      val debugInfo: Option[ServerDebugInfo] = reloadCache.cachedFrom(tryApp).serverDebugInfo
+      ServerDebugInfo.attachToRequestHeader(convertedRequestHeader, debugInfo)
+    }
+    val (taggedRequestHeader, handler) = Server.getHandlerFor(debugInfoRequestHeader, tryApp)
     val responseFuture = executeHandler(
-      newTryApp,
+      tryApp,
       decodedRequest,
       taggedRequestHeader,
       requestBodySource,
@@ -225,25 +233,6 @@ class AkkaHttpServer(
     }
   }
 
-  private def getHandler(
-    requestHeader: RequestHeader, tryApp: Try[Application]
-  ): (RequestHeader, Handler, Try[Application]) = {
-    Server.getHandlerFor(requestHeader, tryApp) match {
-      case Left(futureResult) =>
-        (
-          requestHeader,
-          EssentialAction(_ => Accumulator.done(futureResult)),
-          Failure(new Exception("getHandler returned Result, but not Application"))
-        )
-      case Right((newRequestHeader, handler, newApp)) =>
-        (
-          newRequestHeader,
-          handler,
-          Success(newApp) // TODO: Change getHandlerFor to use the app that we already had
-        )
-    }
-  }
-
   private def executeHandler(
     tryApp: Try[Application],
     request: HttpRequest,

framework/src/play-integration-test/src/test/scala/play/it/server/ServerReloadingSpec.scala

@@ -15,6 +15,7 @@ import play.api.routing.sird._
 import play.api.test.{ PlaySpecification, WsTestClient }
 import play.api.{ Application, Configuration }
 import play.core.ApplicationProvider
+import play.core.server.common.ServerDebugInfo
 import play.core.server.{ ServerConfig, ServerProvider }
 import play.it.{ AkkaHttpIntegrationSpecification, NettyIntegrationSpecification, ServerIntegrationSpecification }
 
@@ -168,6 +169,56 @@ trait ServerReloadingSpec extends PlaySpecification with WsTestClient with Serve
       }
     }
 
+    "only reload its configuration when the application changes" in {
+
+      val testAppProvider = new TestApplicationProvider
+      withApplicationProvider(testAppProvider) { implicit port: Port =>
+
+        def appWithConfig(conf: (String, Any)*): Success[Application] = {
+          Success(GuiceApplicationBuilder()
+            .configure(conf: _*)
+            .overrides(bind[Router].toProvider[ServerReloadingSpec.TestRouterProvider])
+            .build())
+        }
+
+        val app1 = appWithConfig("play.server.debug.addDebugInfoToRequests" -> true)
+        testAppProvider.provide(app1)
+        await(wsUrl("/getserverconfigcachereloads").get()).body must_== "Some(1)"
+        await(wsUrl("/getserverconfigcachereloads").get()).body must_== "Some(1)"
+
+        val app2 = Failure(new Exception())
+        testAppProvider.provide(app2)
+        await(wsUrl("/getserverconfigcachereloads").get()).status must_== 500
+        await(wsUrl("/getserverconfigcachereloads").get()).status must_== 500
+
+        val app3 = appWithConfig("play.server.debug.addDebugInfoToRequests" -> true)
+        testAppProvider.provide(app3)
+        await(wsUrl("/getserverconfigcachereloads").get()).body must_== "Some(3)"
+        await(wsUrl("/getserverconfigcachereloads").get()).body must_== "Some(3)"
+
+        val app4 = appWithConfig()
+        testAppProvider.provide(app4)
+        await(wsUrl("/getserverconfigcachereloads").get()).body must_== "None"
+        await(wsUrl("/getserverconfigcachereloads").get()).body must_== "None"
+
+        val app5 = appWithConfig("play.server.debug.addDebugInfoToRequests" -> true)
+        testAppProvider.provide(app5)
+        await(wsUrl("/getserverconfigcachereloads").get()).body must_== "Some(5)"
+        await(wsUrl("/getserverconfigcachereloads").get()).body must_== "Some(5)"
+
+        val app6 = Failure(new Exception())
+        testAppProvider.provide(app6)
+        await(wsUrl("/getserverconfigcachereloads").get()).status must_== 500
+        await(wsUrl("/getserverconfigcachereloads").get()).status must_== 500
+
+        val app7 = appWithConfig("play.server.debug.addDebugInfoToRequests" -> true)
+        testAppProvider.provide(app7)
+        await(wsUrl("/getserverconfigcachereloads").get()).body must_== "Some(7)"
+        await(wsUrl("/getserverconfigcachereloads").get()).body must_== "Some(7)"
+
+      }
+    }
+
   }
 }
 
@@ -187,6 +238,10 @@ private[server] object ServerReloadingSpec {
       case GET(p"/getremoteaddress") => action { request: Request[_] =>
         Results.Ok(request.remoteAddress)
       }
+      case GET(p"/getserverconfigcachereloads") => action { request: Request[_] =>
+        val reloadCount: Option[Int] = request.attrs.get(ServerDebugInfo.Attr).map(_.serverConfigCacheReloads)
+        Results.Ok(reloadCount.toString)
+      }
     }
   }
 }

framework/src/play-netty-server/src/main/scala/play/core/server/netty/PlayRequestHandler.scala

@@ -18,11 +18,9 @@ import play.api.http._
 import play.api.libs.streams.Accumulator
 import play.api.mvc._
 import play.api.{ Application, Logger }
-import play.core.ApplicationProvider
 import play.core.server.{ NettyServer, Server }
-import play.core.server.common.{ ReloadCache, ServerResultUtils }
+import play.core.server.common.{ ReloadCache, ServerDebugInfo, ServerResultUtils }
 
-import scala.compat.java8.FutureConverters
 import scala.concurrent.Future
 import scala.util.{ Failure, Success, Try }
 
@@ -48,7 +46,8 @@ private[play] class PlayRequestHandler(val server: NettyServer, val serverHeader
    */
   private case class ReloadCacheValues(
       resultUtils: ServerResultUtils,
-      modelConversion: NettyModelConversion
+      modelConversion: NettyModelConversion,
+      serverDebugInfo: Option[ServerDebugInfo]
   )
 
   /**
@@ -61,7 +60,8 @@ private[play] class PlayRequestHandler(val server: NettyServer, val serverHeader
       val modelConversion = new NettyModelConversion(serverResultUtils, forwardedHeaderHandler, serverHeader)
       ReloadCacheValues(
         resultUtils = serverResultUtils,
-        modelConversion = modelConversion
+        modelConversion = modelConversion,
+        serverDebugInfo = reloadDebugInfo(tryApp, NettyServer.provider)
       )
     }
   }
@@ -81,43 +81,43 @@ private[play] class PlayRequestHandler(val server: NettyServer, val serverHeader
     import play.core.Execution.Implicits.trampoline
 
     val tryApp: Try[Application] = server.applicationProvider.get
+    val cacheValues: ReloadCacheValues = reloadCache.cachedFrom(tryApp)
 
-    val tryRequest: Try[RequestHeader] = modelConversion(tryApp).convertRequest(channel, request)
+    val tryRequest: Try[RequestHeader] = cacheValues.modelConversion.convertRequest(channel, request)
 
-    def clientError(statusCode: Int, message: String) = {
+    // Helper to attach ServerDebugInfo attribute to a RequestHeader
+    def attachDebugInfo(rh: RequestHeader): RequestHeader = {
+      ServerDebugInfo.attachToRequestHeader(rh, cacheValues.serverDebugInfo)
+    }
+
+    def clientError(statusCode: Int, message: String): (RequestHeader, Handler) = {
       val unparsedTarget = modelConversion(tryApp).createUnparsedRequestTarget(request)
       val requestHeader = modelConversion(tryApp).createRequestHeader(channel, request, unparsedTarget)
-      val result = errorHandler(server.applicationProvider.current).onClientError(requestHeader, statusCode,
+      val debugHeader = attachDebugInfo(requestHeader)
+      val result = errorHandler(tryApp).onClientError(debugHeader, statusCode,
         if (message == null) "" else message)
       // If there's a problem in parsing the request, then we should close the connection, once done with it
-      requestHeader -> Left(result.map(_.withHeaders(HeaderNames.CONNECTION -> "close")))
+      debugHeader -> Server.actionForResult(result.map(_.withHeaders(HeaderNames.CONNECTION -> "close")))
     }
 
-    val (requestHeader, resultOrHandler) = tryRequest match {
-
+    val (requestHeader, handler): (RequestHeader, Handler) = tryRequest match {
       case Failure(exception: TooLongFrameException) => clientError(Status.REQUEST_URI_TOO_LONG, exception.getMessage)
       case Failure(exception) => clientError(Status.BAD_REQUEST, exception.getMessage)
       case Success(untagged) =>
-        Server.getHandlerFor(untagged, tryApp) match {
-
-          case Left(directResult) =>
-            untagged -> Left(directResult)
-
-          case Right((taggedRequestHeader, handler, application)) =>
-            taggedRequestHeader -> Right((handler, application))
-        }
-
+        val debugHeader: RequestHeader = attachDebugInfo(untagged)
+        Server.getHandlerFor(debugHeader, tryApp)
     }
 
-    resultOrHandler match {
+    handler match {
 
       //execute normal action
-      case Right((action: EssentialAction, app)) =>
-        handleAction(action, requestHeader, request, Some(app))
+      case action: EssentialAction =>
+        handleAction(action, requestHeader, request, tryApp)
 
-      case Right((ws: WebSocket, app)) if requestHeader.headers.get(HeaderNames.UPGRADE).exists(_.equalsIgnoreCase("websocket")) =>
+      case ws: WebSocket if requestHeader.headers.get(HeaderNames.UPGRADE).exists(_.equalsIgnoreCase("websocket")) =>
         logger.trace("Serving this request with: " + ws)
 
+        val app = tryApp.get // Guaranteed to be Success for a WebSocket handler
         val wsProtocol = if (requestHeader.secure) "wss" else "ws"
         val wsUrl = s"$wsProtocol://${requestHeader.host}${requestHeader.path}"
         val bufferLimit = app.configuration.getDeprecated[ConfigMemorySize]("play.server.websocket.frame.maxLength", "play.websocket.buffer.limit").toBytes.toInt
@@ -130,7 +130,7 @@ private[play] class PlayRequestHandler(val server: NettyServer, val serverHeader
           case Left(result) =>
             // WebSocket was rejected, send result
             val action = EssentialAction(_ => Accumulator.done(result))
-            handleAction(action, requestHeader, request, Some(app))
+            handleAction(action, requestHeader, request, tryApp)
           case Right(flow) =>
             import app.materializer
             val processor = WebSocketHandler.messageFlowToFrameProcessor(flow, bufferLimit)
@@ -141,32 +141,26 @@ private[play] class PlayRequestHandler(val server: NettyServer, val serverHeader
           case error =>
             app.errorHandler.onServerError(requestHeader, error).flatMap { result =>
               val action = EssentialAction(_ => Accumulator.done(result))
-              handleAction(action, requestHeader, request, Some(app))
+              handleAction(action, requestHeader, request, tryApp)
             }
         }
 
       //handle bad websocket request
-      case Right((ws: WebSocket, app)) =>
+      case ws: WebSocket =>
         logger.trace("Bad websocket request")
         val action = EssentialAction(_ => Accumulator.done(
           Results.Status(Status.UPGRADE_REQUIRED)("Upgrade to WebSocket required").withHeaders(
             HeaderNames.UPGRADE -> "websocket",
             HeaderNames.CONNECTION -> HeaderNames.UPGRADE
           )
         ))
-        handleAction(action, requestHeader, request, Some(app))
+        handleAction(action, requestHeader, request, tryApp)
 
       // This case usually indicates an error in Play's internal routing or handling logic
-      case Right((h, _)) =>
+      case h =>
         val ex = new IllegalStateException(s"Netty server doesn't handle Handlers of this type: $h")
         logger.error(ex.getMessage, ex)
         throw ex
-
-      case Left(e) =>
-        logger.trace("No handler, got direct result: " + e)
-        val action = EssentialAction(_ => Accumulator.done(e))
-        handleAction(action, requestHeader, request, None)
-
     }
   }
 
@@ -264,12 +258,13 @@ private[play] class PlayRequestHandler(val server: NettyServer, val serverHeader
    * Handle an essential action.
    */
   private def handleAction(action: EssentialAction, requestHeader: RequestHeader,
-    request: HttpRequest, app: Option[Application]): Future[HttpResponse] = {
-    implicit val mat: Materializer = app.fold(server.materializer)(_.materializer)
+    request: HttpRequest, tryApp: Try[Application]): Future[HttpResponse] = {
+    implicit val mat: Materializer = tryApp match {
+      case Success(app) => app.materializer
+      case Failure(_) => server.materializer
+    }
     import play.core.Execution.Implicits.trampoline
 
-    val tryApp = Try(app.get)
-
     // Execute the action on the Play default execution context
     val actionFuture = Future(action(requestHeader))(mat.executionContext)
     for {
@@ -283,24 +278,27 @@ private[play] class PlayRequestHandler(val server: NettyServer, val serverHeader
       }.recoverWith {
         case error =>
           logger.error("Cannot invoke the action", error)
-          errorHandler(app).onServerError(requestHeader, error)
+          errorHandler(tryApp).onServerError(requestHeader, error)
       }
       // Clean and validate the action's result
       validatedResult <- {
         val cleanedResult = resultUtils(tryApp).prepareCookies(requestHeader, actionResult)
-        resultUtils(tryApp).validateResult(requestHeader, cleanedResult, errorHandler(app))
+        resultUtils(tryApp).validateResult(requestHeader, cleanedResult, errorHandler(tryApp))
       }
       // Convert the result to a Netty HttpResponse
       convertedResult <- modelConversion(tryApp)
-        .convertResult(validatedResult, requestHeader, request.protocolVersion(), errorHandler(app))
+        .convertResult(validatedResult, requestHeader, request.protocolVersion(), errorHandler(tryApp))
     } yield convertedResult
   }
 
   /**
    * Get the error handler for the application.
    */
-  private def errorHandler(app: Option[Application]): HttpErrorHandler =
-    app.fold[HttpErrorHandler](DefaultHttpErrorHandler)(_.errorHandler)
+  private def errorHandler(tryApp: Try[Application]): HttpErrorHandler =
+    tryApp match {
+      case Success(app) => app.errorHandler
+      case Failure(_) => DefaultHttpErrorHandler
+    }
 
   /**
    * Sends a simple response with no body, then closes the connection.