Deferred body parsing #9779

mkurz · 2019-10-24T23:38:40Z

By default nothing changes, it's opt-in.
Backwards compatible.
Includes tests.
Includes documentation.

The idea is to store the needed logic (which depends on the different server backends) via a lambda into a request attribute and call it later when needed. Curious what you think about this (I hope I didn't overlook something and this is complete bs. But its working like a charm...)
For the Java API everything is done automatically. For the Scala API you need to call the parseBody method yourself, because we don't have so much control about the Scala actions like we have about the Java ones. (However this also gives Scala users more flexibility, meaning I can sell this as a feature 😉)

Please have a look at the docs I added, I think it explains the motivation behind this pull request. Marcos and I discussed this as well already. I had real world use cases for such feature already, and it looks like others as well (found another one which I didn't save the link for.)

The relevant implementation actually only happens in Action.scala and in the two server backends.

To help reviewing, here are the test results for the integration tests I added.

[info] AkkaHTTPServerDeferredBodyParsingSpec
[info] Scala API should
[info]   + by default not defer body parsing (2 seconds, 576 ms)
[info]   + defer body parsing when activated globally via config (202 ms)
[info]   + not defer body parsing when explicitly deactivated globally via config (203 ms)
[info]   + defer body parsing when deactivated globally but activated via route modifier (143 ms)
[info]   + defer body parsing when deactivated globally but activated via case insensitive route modifier (146 ms)
[info]   + not defer body parsing when activated globally but deactivated via route modifier (139 ms)
[info]   + not defer body parsing when activated globally but deactivated via case insensitive route modifier (172 ms)
[info]   + not defer body parsing when activated globally and also via route modifier but deactivated via route modifier (148 ms)
[info]   + not defer body parsing when deactivated globally and activated via route modifier but deactivated via route modifier (148 ms)
[info] Java API should
[info]   + by default not defer body parsing (242 ms)
[info]   + defer body parsing when activated globally via config (155 ms)
[info]   + not defer body parsing when explicitly deactivated globally via config (160 ms)
[info]   + defer body parsing when deactivated globally but activated via route modifier (168 ms)
[info]   + defer body parsing when deactivated globally but activated via case insensitive route modifier (155 ms)
[info]   + not defer body parsing when activated globally but deactivated via route modifier (164 ms)
[info]   + not defer body parsing when activated globally but deactivated via case insensitive route modifier (168 ms)
[info]   + not defer body parsing when activated globally and also via route modifier but deactivated via route modifier (168 ms)
[info]   + not defer body parsing when deactivated globally and activated via route modifier but deactivated via route modifier (129 ms)
[info] Total for specification AkkaHTTPServerDeferredBodyParsingSpec
[info] Finished in 5 seconds, 791 ms
[info] 18 examples, 20 expectations, 0 failure, 0 error
...
...
...
[info] NettyServerDeferredBodyParsingSpec
[info] Scala API should
[info]   + by default not defer body parsing (4 seconds, 373 ms)
[info]   + defer body parsing when activated globally via config (2 seconds, 374 ms)
[info]   + not defer body parsing when explicitly deactivated globally via config (2 seconds, 437 ms)
[info]   + defer body parsing when deactivated globally but activated via route modifier (2 seconds, 433 ms)
[info]   + defer body parsing when deactivated globally but activated via case insensitive route modifier (2 seconds, 343 ms)
[info]   + not defer body parsing when activated globally but deactivated via route modifier (2 seconds, 405 ms)
[info]   + not defer body parsing when activated globally but deactivated via case insensitive route modifier (2 seconds, 504 ms)
[info]   + not defer body parsing when activated globally and also via route modifier but deactivated via route modifier (2 seconds, 541 ms)
[info]   + not defer body parsing when deactivated globally and activated via route modifier but deactivated via route modifier (2 seconds, 376 ms)
[info] Java API should
[info]   + by default not defer body parsing (2 seconds, 431 ms)
[info]   + defer body parsing when activated globally via config (2 seconds, 339 ms)
[info]   + not defer body parsing when explicitly deactivated globally via config (2 seconds, 321 ms)
[info]   + defer body parsing when deactivated globally but activated via route modifier (2 seconds, 336 ms)
[info]   + defer body parsing when deactivated globally but activated via case insensitive route modifier (2 seconds, 366 ms)
[info]   + not defer body parsing when activated globally but deactivated via route modifier (2 seconds, 345 ms)
[info]   + not defer body parsing when activated globally but deactivated via case insensitive route modifier (2 seconds, 358 ms)
[info]   + not defer body parsing when activated globally and also via route modifier but deactivated via route modifier (2 seconds, 380 ms)
[info]   + not defer body parsing when deactivated globally and activated via route modifier but deactivated via route modifier (2 seconds, 322 ms)
[info] Total for specification NettyServerDeferredBodyParsingSpec
[info] Finished in 45 seconds, 569 ms
[info] 18 examples, 20 expectations, 0 failure, 0 error

Fixes #9634

mkurz · 2019-10-24T23:42:28Z

core/play/src/main/scala/play/api/mvc/Action.scala

+      logger.trace("Not deferring body parsing for request: " + rh)
+      // Run the parser first and then call the action
+      BodyParser.runParserThenInvokeAction(parser, rh, apply)(executionContext)
+    }


I think it's best to start reviewing here in the apply method. Basically when the request attribute exists, we know the parsing is deferred and therefore just skip the body parsing part, but start to execute the action.
The request attribute will be stored by the server backend.

Basically the original code of this apply method just moved to runParserThenInvokeAction.

mkurz · 2019-10-24T23:48:30Z

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

+    } else {
+      taggedRequestHeader
+    }))
+    val resultFuture: Future[Result] = invokeAction(futureAcc, deferBodyParsing)


Here in the server backend we decide if to run the body parser immediately (that would be supplied by the action if the request attribute does not get stored here) or if to skip it (which results in just running the action in Action.scala).

Some notes:
I switched to trampoline because it's a fast Future anyway so that should not really matter and actually the netty server backend also uses trampoline. (So no need to also capture the executioncontext in the lambda)

mkurz · 2019-10-24T23:50:45Z

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

+            if (deferBodyParsing) {
+              acc.run() // don't parse anything
+            } else {
+              val body = modelConversion(tryApp).convertRequestBody(request)


I was thinking about moving this line val body = modelConversion(tryApp).convertRequestBody(request) out of the lambda, and just capture the body instead of the request (to end up like in the akka-http backend to capture the source), but I think that does not matter. Edit: I kept it here to make keep it part of the exception handling.

mkurz · 2019-10-24T23:51:15Z

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

@@ -274,6 +274,11 @@ object Server {

  case object ServerStoppedReason extends CoordinatedShutdown.Reason

+  private[server] def routeModifierDefersBodyParsing(global: Boolean, rh: RequestHeader): Boolean = {


I made this package private, because I don't think someone should mess with it.

Also I added this Attr because both the server backends projects and the play core project need to access/share it.

mkurz · 2019-10-24T23:52:36Z

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

@@ -289,31 +292,48 @@ private[play] class PlayRequestHandler(
      action: EssentialAction,
      requestHeader: RequestHeader,
      request: HttpRequest,
-      tryApp: Try[Application]
+      tryApp: Try[Application],
+      deferredBodyParsingAllowed: Boolean = false


I added this deferredBodyParsingAllowed only to not store the request attribute for the websocket error actions (but I think that would not even matter), just an optimization.

documentation/manual/working/javaGuide/main/http/JavaActionsComposition.md

mkurz · 2019-10-28T09:39:20Z

Summarizing the discussions we had about this feature:

We do not want to pass null as request body when a user opts into deferred body parsing.
Therefore we have to find a way how to not pass null...

Passing Either[RequestHeader, Request[A]] to an Action instead of Request[A]
- Not so good I think because we would have to change too much (ActionBuilders invokeBlock, which should not care about this stuff)
Change def body: A to def body: Option[A] (but that is too generic probably)
Change def body: A to something def body: LazyBody[A] where LazyBody is defined as (option-like) :

sealed trait LazyBody[A]
final case class DeferedBody[A](...?) extends LazyBody[A]
final case class Body[A](value: A) extends LazyBody[A]

Note for this last implementation: I do think that we still need to parse the body explicitly by calling BodyParser.parsebody(...) (and not make some kind of lazy request where you can request the body to be parsed (and memoised)) because parsing needs a next action (it would be weird to always pass a next action to request.body(next) when you want to access the body).

One more thing to take care of when implementing is Java's Http.Request<RequestBody>...

I guess I will go with the LazyBody approach. So deprecate def body: A, introduce def getBody: LazyBody[A]

Some other notes which might be useful:
Running a HEAD request, for Scala req.body gives me AnyContentAsEmpty and for Java it's an instance of play.mvc.Http.RequestBody that contains an empty Optional. So both are not null when a body is empty.

mkurz · 2020-01-27T10:50:27Z

Because I was not sure if storing a lambda (which captures the source) into a request attribute within the server backend (and carrying that object through) effects performance somehow, I did some benchmarking on my machine using the TechEmpower Framework Benchmark project with a local, custom Play build (Where I just rebased this pull request onto the 2.8.0 tag (not branch)).
I did use the play2-java test, because this also implicitly tests the Scala implementation (and the server backend implementations are the same anyway). The most meaningful test IMHO is the plain-text test which just returns a string to the client, without any other calculations like processing JSON, rendering views, etc.
I just added an action annotation onto the controller which passed the request through. I did test runs for each server backend as well, to make sure we don't overlook performance problems e.g. in the netty backend, even though the akka-http one may look nice.
After several runs I could not see any significant performance differences between original Play 2.8.0, my custom 2.8.0 build with deferred body parsing deactivated and custom 2.8.0 with deferred body parsing activated. If any, there maybe was 1 - 1,5 % difference, but I would count that as benchmark "noise". (The test of course are never the same, but very very close each run).

So performance wise I have a very good feeling we shouldn't have an issue here.

bbatarelo · 2020-06-07T19:19:22Z

Do we know what the status of this feature is? It is a very valuable one for some of us :)

mkurz · 2020-06-08T07:53:25Z

@bbatarelo My goal is to finish this pull request before Play 2.9 will be released. Also for me this feature is very useful. It is very high up on my TODO list.
Just curious, why do you need this feature? What do you want to use it for?

bbatarelo · 2020-06-08T09:30:58Z

@mkurz exactly as the original issue of the ticket: preventing transparent alpakka streaming of multipart content to s3 in case authentication and/or other prerequisites aren't satisfied.
Also, it just seems more natural approach to me in general, if nothing then for potential performance reasons in case of heavy bodies.

mkurz · 2020-06-08T09:35:52Z

@bbatarelo Good to hear others have the same thoughts and requirements like I do 😉 I definitely will push forward this pull request to include it into 2.9.

What do you think if I provide a custom 2.8 release with this feature included (based on the current 2.8.2 release) Would you like to use such a custom release? It would probably be good for testing this feature (I think it's stable anyway).

bbatarelo · 2020-06-08T09:37:45Z

If it's not cumbersome to use meaning if I only need to change play plugin version or something like that, I'd start using it right away :)

mkurz · 2020-06-08T09:48:30Z

@bbatarelo No it's not cumbersome at all. You will just have to change the Play version to something like 2.8.2-DBP (for Deferred Body Parsing) and you will have to add a new resolver which is just one line:

resolvers += Resolver.url("Custom Play releases by mkurz", url("https://mkurz.github.io/..."))(Resolver.ivyStylePatterns)

I will build that custom release later today and let you know the details. Are you using Scala 2.13?

bbatarelo · 2020-06-08T09:50:03Z

That's good, thank you. Yes, I'm on 2.13.

mkurz · 2020-06-09T09:28:48Z

@bbatarelo It's done. I created a custom 2.8.2-DBP release. It's just the latest Play 2.8.2 release (commit bc1badd) + this pull request here backported: https://github.com/mkurz/playframework/commits/2.8.x-DBP

What you have to do (simple):
In your project/plugins.sbt you have to use 2.8.2-DBP instead of just 2.8.2:

addSbtPlugin("com.typesafe.play" % "sbt-plugin" % "2.8.2-DBP")

And you also have to add a resolver to both project/plugins.sbt and build.sbt:

resolvers += Resolver.url("Custom Play releases by mkurz", url("https://mkurz.github.io/playframework/releases/"))(Resolver.ivyStylePatterns)

That's it!

Now you can enabled deferred body parsing in conf/application.conf:

play.server.deferBodyParsing = true

Let me know if that works for you. Should be stable.
Just curious: Do you use PlayJava or PlayScala?

tsuyoshizawa · 2023-08-16T09:07:16Z

core/play-integration-test/src/it/java/play/it/http/parsingdeferred/Attrs.java

@@ -0,0 +1,11 @@
+/*
+ * Copyright (C) Lightbend Inc. <https://www.lightbend.com>


Current copyright is below? 👀

Copyright (C) from 2022 The Play Framework Contributors https://github.com/playframework, 2011-2021 Lightbend Inc. https://www.lightbend.com

I know, formatting still missing, I am currently testing this PR locally again and want to merge it at the absolutly last PR for 2.9 since I don't want to hold this feature back anymore and specially in the light of #11170 (and given the PR was finished basically anyway except)

Roger that! I saw you force push and wondered if you were close to merging.

Co-Authored-By: Ignasi Marimon-Clos <ignasi@lightbend.com>

…on trampoline

When deferring is disabled (the default) we shouldn't use a lambda but just a normal method because it's faster (than lambdas/invokedynamic). Also akka-http performs much better with the ec of the materializer

mkurz · 2023-08-18T17:36:52Z

So this is done. Actually the PR was always done more or less. Back in 2019 it was almost merged the evening before Play 2.8, Marcos almost wanted to merge but since they didn't want to include new features anymore they skipped it. Only Dale had a small concern that returning null isn't really nice. However, this is a thing we could improve in an upcoming PRs if people think that is necessary and wouldn't change this PR anyway. Since the default stays as it is and we can enhance things later this is neglectable at this point IMHO.

I wanted to get this in personally a long time since I found it very useful for projects I worked on back then and I do not see a reason to hold this back any longer.
It's all tested, yesterday I even did exhaustive testing and benchmarking to make sure this isn't slowing anything down because I was worried about that back then. I fixed some performance issues so now that benchmarks of main and 2.8.20 are the same. (BTW: Just switching to Java 17 will automatically improve performance of you Play apps).

During reviewing/finishing this PR I ran into three other things:

This is is.

mkurz added this to the Play 2.8.0-RC1 milestone Oct 24, 2019

mkurz commented Oct 24, 2019

View reviewed changes

mkurz force-pushed the deferBodyParsing branch 2 times, most recently from 2d360f2 to 78331e1 Compare October 25, 2019 00:28

This comment was marked as resolved.

Sign in to view

mkurz requested a review from dwijnand October 25, 2019 06:38

ignasi35 reviewed Oct 25, 2019

View reviewed changes

documentation/manual/working/javaGuide/main/http/JavaActionsComposition.md Outdated Show resolved Hide resolved

dwijnand modified the milestones: Play 2.8.0-RC1, Play 2.8.0 optional changes Oct 25, 2019

mkurz added the status:block-merge label Oct 28, 2019

mkurz force-pushed the deferBodyParsing branch 3 times, most recently from 5806e99 to 622cc23 Compare January 23, 2020 23:23

dwijnand removed their request for review January 24, 2020 08:21

mkurz force-pushed the deferBodyParsing branch from 622cc23 to 77ff489 Compare June 8, 2020 23:29

mkurz force-pushed the deferBodyParsing branch from fcfa28d to ccd38fd Compare August 15, 2023 15:40

tsuyoshizawa reviewed Aug 16, 2023

View reviewed changes

mkurz and others added 8 commits August 16, 2023 22:29

Deferred body parsing

7af22dc

Move DeferredBodyParserInvoker to play.api.mvc.request.RequestAttrKey

a456b49

Fix bad wording

ce8f700

Co-Authored-By: Ignasi Marimon-Clos <ignasi@lightbend.com>

Formatting

409b06e

Remove parseBody() from Action and ActionBuilder for now

22d72bb

Mention request.hasBody

ddee4d6

MiMa filters

f8c18e0

Formatting

6cfe730

mkurz force-pushed the deferBodyParsing branch from ccd38fd to 6cfe730 Compare August 16, 2023 20:30

mkurz added 7 commits August 17, 2023 18:14

Test should run on correct port

e08a1d0

Make tests work with Scala 3

ac8d29d

Also in Java body should be parsed on default execution context, not …

1f9fc7a

…on trampoline

Better log message

046f85e

Performance optimizations

7c78f1a

When deferring is disabled (the default) we shouldn't use a lambda but just a normal method because it's faster (than lambdas/invokedynamic). Also akka-http performs much better with the ec of the materializer

Move highlights into 2.9

57f55ef

Seems MiMa learned this meanwhile ;)

831920a

mkurz force-pushed the deferBodyParsing branch from f7ad9a8 to 831920a Compare August 17, 2023 16:39

Formatting + headers

613d3cc

mkurz force-pushed the deferBodyParsing branch from cc8308f to 613d3cc Compare August 17, 2023 17:21

mkurz removed the status:block-merge label Aug 17, 2023

mkurz added 3 commits August 18, 2023 12:18

Rename request variable and make it accessable

343ffc4

Maybe nicer

be4451a

Update docs because things changed meanwhile

1432ff4

mkurz merged commit 7daa337 into playframework:main Aug 18, 2023
28 checks passed

mkurz deleted the deferBodyParsing branch August 18, 2023 17:37

mkurz modified the milestones: 2.10.0, 2.9.0 Aug 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deferred body parsing #9779

Deferred body parsing #9779

mkurz commented Oct 24, 2019 •

edited

mkurz Oct 24, 2019

mkurz Oct 24, 2019 •

edited

mkurz Oct 24, 2019 •

edited

mkurz Oct 24, 2019

mkurz Oct 24, 2019

mkurz Oct 24, 2019

This comment was marked as resolved.

This comment was marked as resolved.

mkurz commented Oct 28, 2019

mkurz commented Jan 27, 2020

bbatarelo commented Jun 7, 2020 •

edited

mkurz commented Jun 8, 2020

bbatarelo commented Jun 8, 2020 •

edited

mkurz commented Jun 8, 2020

bbatarelo commented Jun 8, 2020

mkurz commented Jun 8, 2020 •

edited

bbatarelo commented Jun 8, 2020

mkurz commented Jun 9, 2020

tsuyoshizawa Aug 16, 2023

mkurz Aug 16, 2023

tsuyoshizawa Aug 16, 2023

mkurz commented Aug 18, 2023 •

edited

		@@ -274,6 +274,11 @@ object Server {

		case object ServerStoppedReason extends CoordinatedShutdown.Reason

		private[server] def routeModifierDefersBodyParsing(global: Boolean, rh: RequestHeader): Boolean = {

		@@ -0,0 +1,11 @@
		/*
		* Copyright (C) Lightbend Inc. <https://www.lightbend.com>

Deferred body parsing #9779

Deferred body parsing #9779

Conversation

mkurz commented Oct 24, 2019 • edited

mkurz Oct 24, 2019

Choose a reason for hiding this comment

mkurz Oct 24, 2019 • edited

Choose a reason for hiding this comment

mkurz Oct 24, 2019 • edited

Choose a reason for hiding this comment

mkurz Oct 24, 2019

Choose a reason for hiding this comment

mkurz Oct 24, 2019

Choose a reason for hiding this comment

mkurz Oct 24, 2019

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

mkurz commented Oct 28, 2019

mkurz commented Jan 27, 2020

bbatarelo commented Jun 7, 2020 • edited

mkurz commented Jun 8, 2020

bbatarelo commented Jun 8, 2020 • edited

mkurz commented Jun 8, 2020

bbatarelo commented Jun 8, 2020

mkurz commented Jun 8, 2020 • edited

bbatarelo commented Jun 8, 2020

mkurz commented Jun 9, 2020

tsuyoshizawa Aug 16, 2023

Choose a reason for hiding this comment

mkurz Aug 16, 2023

Choose a reason for hiding this comment

tsuyoshizawa Aug 16, 2023

Choose a reason for hiding this comment

mkurz commented Aug 18, 2023 • edited

mkurz commented Oct 24, 2019 •

edited

mkurz Oct 24, 2019 •

edited

mkurz Oct 24, 2019 •

edited

bbatarelo commented Jun 7, 2020 •

edited

bbatarelo commented Jun 8, 2020 •

edited

mkurz commented Jun 8, 2020 •

edited

mkurz commented Aug 18, 2023 •

edited