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

Run Actions in their ExecutionContext #8225

Open
richdougherty opened this issue Feb 14, 2018 · 4 comments
Open

Run Actions in their ExecutionContext #8225

richdougherty opened this issue Feb 14, 2018 · 4 comments
Assignees

Comments

@richdougherty
Copy link
Member

Due to several changes in how we handle threading, Action code no longer always runs in the action's ExecutionContext.

The Action runs in the default dispatcher:

runAction(applicationProvider.get, request, taggedRequestHeader, requestBodySource,
action, errorHandler)(actorSystem.dispatcher)
}
private[play] def runAction(
tryApp: Try[Application],
request: HttpRequest,
taggedRequestHeader: RequestHeader,
requestBodySource: Either[ByteString, Source[ByteString, _]],
action: EssentialAction,
errorHandler: HttpErrorHandler)(implicit ec: ExecutionContext): Future[HttpResponse] = {
val futureAcc: Future[Accumulator[ByteString, Result]] = Future(action(taggedRequestHeader))

Also an Action is usually wrapped in a series of filters:

// 1. Query the router to get a handler
// 2. Resolve handlers that preprocess the request
// 3. Modify the handler to do filtering, if necessary
// 4. Again resolve any handlers that do preprocessing
val routedHandler = routeWithFallback(request)
val (preprocessedRequest, preprocessedHandler) = Handler.applyStages(request, routedHandler)
val filteredHandler = filterHandler(preprocessedRequest, preprocessedHandler)
val (preprocessedPreprocessedRequest, preprocessedFilteredHandler) = Handler.applyStages(preprocessedRequest, filteredHandler)
(preprocessedPreprocessedRequest, preprocessedFilteredHandler)

The action uses Accumulator.mapFuture to call its logic:

def apply(rh: RequestHeader): Accumulator[ByteString, Result] = parser(rh).mapFuture {
case Left(r) =>
logger.trace("Got direct result from the BodyParser: " + r)
Future.successful(r)
case Right(a) =>
val request = Request(rh, a)
logger.trace("Invoking action with request: " + request)
apply(request)
}(executionContext)
/**
* The execution context to run this action in
*
* @return The execution context to run the action in
*/
def executionContext: ExecutionContext

But for performance reasons this only uses the Action's ExecutionContext for a SinkAccumulator, not a StrictAccumulator:

def mapFuture[B](f: A => Future[B])(implicit executor: ExecutionContext): Accumulator[E, B] =
new SinkAccumulator(sink.mapMaterializedValue(_.flatMap(f)))

def mapFuture[B](f: A => Future[B])(implicit executor: ExecutionContext): Accumulator[E, B] =
mapMat { future =>
future.value match {
// optimize already completed case
case Some(Success(a)) =>
Try(f(a)) match {
case Success(fut) => fut
case Failure(ex) => Future.failed(ex)
}
case _ => future.flatMap(f)
}
}

Since it's such a big performance win to avoid a thread-switch when using a StrictAccumulator, I think we want to stick with the current behaviour in Action. (See also: #8224)

One approach may be to change the code in AkkaHttpServer (and the Netty PlayRequestHandler) to run the action and filter code in the correct ExecutionContext when the action is first called, before the body is even parsed. In other words the server should somehow figure out the action's ExecutionContext and use that value when it dispatches the request. This means that the action will run in the right ExecutionContext even if the action has a strict body.

It's a bit tricky to figure out the Action's ExecutionContext since the server may not have a direct reference to the ultimate Action objeect. The server only has a reference to an EssentialAction, which is usually constructed from be a series of Filters wrapping an Action.

We need to figure out a way to get the ExecutionContext for an Action even if wrapped by some filters. This potentially will require a change to the HttpRequestHandler or Handler APIs, but we should try to make changes minimal.

Another approach is to ask the HttpRequestHandler to handle the details of ExecutionContext switching. For example, the filterAction method could return an EssentialAction that runs the Filters and Action in the Action's ExecutionContext.

/**
* Apply filters to the given action.
*/
protected def filterAction(next: EssentialAction): EssentialAction = {
filters.foldRight(next)(_ apply _)
}

The HttpRequestHandler might be the most appropriate place to put the logic, since it know about Filters and Actions. It does come at the cost of more complexity in the HttpRequetHandler, but I think that's where we should be putting more request processing logic anyway, since it abstracts out the details that are shared between server backends. See: #8005.

@richdougherty richdougherty self-assigned this Feb 14, 2018
@richdougherty richdougherty added this to the Play 2.6.x milestone Feb 14, 2018
@domdorn
Copy link
Member

domdorn commented Mar 4, 2018

I've noticed that this is happening especially when you have a mixed java/scala project. E.g. some controllers are in Java, others are in Scala and especially if the Custom HttpRequestHandler is written in Scala (even when its copied exactly from the documentations VirtualHostRequestHandler)

@richdougherty
Copy link
Member Author

Thanks for sharing your experience @domdorn. Once we've got a patch in it would be great if you could test it to see if it's an improvement for you. 😄

@richdougherty
Copy link
Member Author

I created an issue for HttpRequestHandler API improvements: #8278. We should keep this in mind while fixing this issue.

@richdougherty
Copy link
Member Author

FYI I have been working on this: playframework:e6ed774...richdougherty:2d0b61e

@dwijnand dwijnand removed this from the Play 2.6.x milestone Jan 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants