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

richdougherty · 2018-04-04T10:47:38Z

The caching of the ReloadCache uses reference equality so we need to be more careful when passing around the Try[Application] returned by ApplicationProvider.get. We need to make sure we keep the same reference.

Also did some refactoring of code while I was making these changes.

richdougherty · 2018-04-04T10:47:59Z

2.6.x version of this PR: #8334.

richdougherty · 2018-04-04T10:49:00Z

I couldn't work out a way to unit test configuration loading without adding intrusive instrumentation into the server code so I plan to verify this by hand before merging. 🤷‍♂️

richdougherty · 2018-04-04T10:50:12Z

framework/src/play-server/src/main/scala/play/core/server/Server.scala

-   * NOTE: This will use the ApplicationProvider of the server to get the application instance.
-   *       Use {@code Server.getHandlerFor(request, tryApp)} to pass a specific application instance
-   */
-  @deprecated("Use Server.getHandlerFor(request, tryApp) instead", "2.7.0")


As of #8334 this method will be deprecated in 2.6.13. The method is highly unlikely to be used outside Play so I think it's OK to remove it for 2.7.0.

👍

Anyway, maybe is worth to add a note in Migration27.md.

richdougherty · 2018-04-04T10:51:40Z

framework/src/play-server/src/main/scala/play/core/server/Server.scala

    request: RequestHeader,
    tryApp: Try[Application]
-  ): Either[Future[Result], (RequestHeader, Handler, Application)] = {
+  ): (RequestHeader, Handler) = {


It's easier to return a Handler than an Either[Future[Result], Handler], since the returned Future[Result] is always just turned into a Handler anyway...

Also the returned Application is not helpful if we're trying to preserve the reference equality of the Try[Application].

richdougherty · 2018-04-04T10:52:43Z

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

@@ -84,40 +82,31 @@ private[play] class PlayRequestHandler(val server: NettyServer, val serverHeader

    val tryRequest: Try[RequestHeader] = modelConversion(tryApp).convertRequest(channel, request)

-    def clientError(statusCode: Int, message: String) = {
+    def clientError(statusCode: Int, message: String): (RequestHeader, Handler) = {


Quite a lot of code in this PR now uses a (RequestHeader, Handler) pair instead of other types. This is probably something we could represent as a special type at some point. Maybe there's a state monad in here for the RequestHeader...

richdougherty · 2018-04-04T10:53:14Z

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

@@ -264,12 +247,10 @@ 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] = {


This line is the key fix. We're ensuring we pass the original Try[Application] through.

richdougherty · 2018-04-04T10:54:49Z

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

      val unparsedTarget = modelConversion(tryApp).createUnparsedRequestTarget(request)
      val requestHeader = modelConversion(tryApp).createRequestHeader(channel, request, unparsedTarget)
-      val result = errorHandler(server.applicationProvider.current).onClientError(requestHeader, statusCode,


This line is the only behaviour change I made in the refactoring—using tryApp instead of applicationProvider.current—however I think the new code is the correct behaviour.

pettyjamesm · 2018-04-04T13:57:16Z

Flame graphs of running the akka http server show that it has the same or similar problem: configuration being loaded every request. Here's the flame graph referenced, sampled from a server that's been online for 12+ hours, well beyond the initial loading:

marcospereira

LGTM, too.

marcospereira · 2018-04-04T17:47:57Z

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

@@ -225,25 +225,6 @@ class AkkaHttpServer(
    }
  }

-  private def getHandler(


marcospereira · 2018-04-04T17:52:28Z

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

-  private def errorHandler(app: Option[Application]): HttpErrorHandler =
-    app.fold[HttpErrorHandler](DefaultHttpErrorHandler)(_.errorHandler)
+  private def errorHandler(tryApp: Try[Application]): HttpErrorHandler =
+    tryApp.fold[HttpErrorHandler](_ => DefaultHttpErrorHandler, _.errorHandler)


The master branch build will have the same problem with Scala 2.11 (no fold for Try).

Since we will drop Scala 2.11, we can add a comment here to use fold when dropping Scala 2.11.

marcospereira · 2018-04-04T17:54:08Z

framework/src/play-server/src/main/scala/play/core/server/Server.scala

-   * NOTE: This will use the ApplicationProvider of the server to get the application instance.
-   *       Use {@code Server.getHandlerFor(request, tryApp)} to pass a specific application instance
-   */
-  @deprecated("Use Server.getHandlerFor(request, tryApp) instead", "2.7.0")


👍

Anyway, maybe is worth to add a note in Migration27.md.

richdougherty · 2018-04-05T02:07:57Z

I couldn't work out a way to unit test configuration loading without adding intrusive instrumentation into the server code so I plan to verify this by hand before merging.

Due to @pettyjamesm's report of issues with Akka HTTP in addition to known issues (#7812) with Netty I've decided to be more conservative and there is now instrumentation in the server and automated tests to check reloads.

The instrumentation is enabled with a configuration setting so it can be used during testing but is disabled in production. Instrumentation details are passed as a request attribute. This is one of those times it would be nice to have JMX hooks so we can view internal server stats. Using a request attribute is the next best thing though.

Reload counts now appear to be correct on both backends.

richdougherty · 2018-04-05T10:01:45Z

Got this probably transient failure on one test, restarting.

[error]   x Not reap a non-expired file
[error]    List(/tmp/8627293184609446381/notcollected.txt) is not empty (TemporaryFileReaperSpec.scala:79)

richdougherty · 2018-04-05T11:02:33Z

I've run the the HelloWorld microbenchmark before and after this change and the change does seem to improve performance.

gmethvin

lgtm except a few suggestions on the migration guide

gmethvin · 2018-04-13T23:38:56Z

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

@@ -92,3 +92,7 @@ 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).
+
+### `Server.getHandlerFor` has been removed


I would write Server#getHandlerFor which is the common notation for instance methods.

gmethvin · 2018-04-13T23:40:10Z

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

+
+### `Server.getHandlerFor` has been removed
+
+This 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.


You should either mention that the new method is private[server] or not mention it at all since it's not public API.

I would mention it. Last time I tried to figure out new relic's tracing logic, this was a "method of interest" and changing the semantics around it might have broken some visibility for a significant number of users, especially for a patch release.

Yeah, fine with me, as long as we note that it's not public API.

I'll make an "internal API changes" section in the migration documentation and we'll try to mention any notable changes here. No guarantees though because we do make quite a few internal changes and it'll be hard to track everything.

Fixes playframework#7812. The caching of the ReloadCache uses reference equality so we need to be more careful when passing around the Try[Application] returned by ApplicationProvider.get. We need to make sure we keep the same reference. Also did some refactoring of code while I was making these changes.

richdougherty · 2018-04-18T19:29:27Z

Migration guide updated, merging.

richdougherty mentioned this pull request Apr 4, 2018

[2.6.x]: Stop Netty server reloading configuration on each request #8334

Merged

richdougherty commented Apr 4, 2018

View reviewed changes

marcospereira changed the title ~~Stop Netty server reloading configuration on each request~~ [master]: Stop Netty server reloading configuration on each request Apr 4, 2018

marcospereira reviewed Apr 4, 2018

View reviewed changes

richdougherty force-pushed the netty-conf-reload branch from ea3f937 to 67331f3 Compare April 9, 2018 22:46

gmethvin approved these changes Apr 13, 2018

View reviewed changes

richdougherty added 7 commits April 18, 2018 15:47

Don't use Try.fold as it's not available in Scala 2.11

a65a5e8

Add migration note

9e7f582

Instrument server reloads, add tests and fix buggy reloads

2302ca0

Add deprecation to ApplicationProvider.current

aeb3bae

Add MiMa filters

8090e63

Migration guide changes

09a10f4

richdougherty force-pushed the netty-conf-reload branch from 7537311 to 09a10f4 Compare April 18, 2018 03:51

richdougherty merged commit a6b5970 into playframework:master Apr 18, 2018

richdougherty mentioned this pull request Apr 18, 2018

[2.6.3] NumberFormatExceptions on every incoming query #7812

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

richdougherty commented Apr 4, 2018

richdougherty commented Apr 4, 2018

richdougherty commented Apr 4, 2018

richdougherty Apr 4, 2018

marcospereira Apr 4, 2018

richdougherty Apr 4, 2018

richdougherty Apr 4, 2018

richdougherty Apr 4, 2018 •

edited

richdougherty Apr 4, 2018

pettyjamesm commented Apr 4, 2018 •

edited

marcospereira left a comment

marcospereira Apr 4, 2018

marcospereira Apr 4, 2018

marcospereira Apr 4, 2018

richdougherty commented Apr 5, 2018 •

edited

richdougherty commented Apr 5, 2018

richdougherty commented Apr 5, 2018

gmethvin left a comment

gmethvin Apr 13, 2018

richdougherty Apr 16, 2018

gmethvin Apr 13, 2018

pettyjamesm Apr 14, 2018

gmethvin Apr 14, 2018

richdougherty Apr 16, 2018

richdougherty commented Apr 18, 2018


		### `Server.getHandlerFor` has been removed

		This 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.

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

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

Conversation

richdougherty commented Apr 4, 2018

richdougherty commented Apr 4, 2018

richdougherty commented Apr 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richdougherty Apr 4, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pettyjamesm commented Apr 4, 2018 • edited

marcospereira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richdougherty commented Apr 5, 2018 • edited

richdougherty commented Apr 5, 2018

richdougherty commented Apr 5, 2018

gmethvin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richdougherty commented Apr 18, 2018

richdougherty Apr 4, 2018 •

edited

pettyjamesm commented Apr 4, 2018 •

edited

richdougherty commented Apr 5, 2018 •

edited