Netty pipeline needs an executionHandler #839

Merged
merged 1 commit into from Apr 8, 2013

Conversation

Projects
None yet
6 participants
Owner

richdougherty commented Apr 4, 2013

When making remote actor calls from an action (which runs on a Netty thread), Netty will throw an exception if that remote message send triggers the creation of a new Akka remote connection. The solution is to use an executionHandler in the netty pipeline, which I believe was done in Play 2.0 (fixing this very same bug). Why was the fix removed in 2.1?

Originally reported by @rkuhn.
@gissues:{"order":93.75,"status":"inprogress"}

benqua commented Mar 16, 2013

Is there a work-around?

rkuhn commented Mar 16, 2013

yes: sending to the remote actor from a different thread, most easily accomplished by wrapping it inside a Future { ... }

benqua commented Mar 16, 2013

Thanks.

@ghost ghost assigned richdougherty Mar 25, 2013

Owner

richdougherty commented Mar 26, 2013

Previous fix:

https://www.assembla.com/spaces/akka/tickets/1452
8c38cde

Looks like the change was lost in another commit, 5 days later:

https://github.com/playframework/Play20/blob/4f24824d14c4b7418fddb9be526f444a37021b6f/framework/src/play/src/main/scala/play/core/server/NettyServer.scala

That commit was meant to be a directory reshuffle, not sure why the ExecutionHandler fix was removed.

Owner

jroper commented Mar 26, 2013

Hey Rich,

Thanks for that work. I took a look, here's the commit that removed it:

851de9f

The issue was not that Play actions are handled on Netty threads, that is definitely not true or even possible with Plays architecture, and we have an integration test that ensures that actions are executing on the thread we expect them to execute.

The issue was that in dev mode, each request can potentially result in the entire application being thrown out (classloader and all) and being reloaded, resulting in Play to start up again. The stack trace in the original issue posted on assembla was from Global.onStart, ie, Plays callback for user code to do initialisation when Play starts up. This was being done on the Netty thread, which it shouldn't be.

The original fix, which delegated all requests to another thread, was wrong, we shouldn't dispatch every request to another thread just because in dev mode the one request that causes reload does something wrong, and that's most probably why @guillaumebort backed it out, and instead dispatched just the application reloading to another thread. That was then removed, most likely accidentally, when a refactoring of Plays thread management was done during 2.1 development, by this commit:

767ba2f#framework/src/play/src/main/scala/play/core/system/ApplicationProvider.scala

@rkuhn Obviously there must be some issue that caused you to discover this regression, do you have the stack trace? I'd just like to confirm that it's from Plays initialisation in dev mode, not from an action.

Collaborator

huntc commented Mar 26, 2013

I question whether the previous fix is necessarily what we want i.e. all response handling would be performed on a separate set of threads to Netty. While this fixes the particular issue reported, it will introduce the overhead of a context switch and possibly delayed execution of the response given the pool allocated to the execution handler. Not every action should be considered long running.

Owner

richdougherty commented Mar 26, 2013

Thanks @jroper, I think you've given me all the clues I need to make the fix. I'll wait to get confirmation from @rkuhn that the problem is with dev mode.

@huntc, I'm guessing you commented before reading James's comment?

Collaborator

huntc commented Mar 26, 2013

@richdougherty indeed. :-)

Owner

richdougherty commented Apr 4, 2013

I was able to reproduce the problem by starting an ActorSystem from the Global object.

https://gist.github.com/richdougherty/5259338

play2-master #996 SUCCESS
This pull request looks good

rkuhn commented Apr 4, 2013

Yes, sorry for the delay: the case which triggered my report looked indeed like a reload in dev mode.

huntc added a commit that referenced this pull request Apr 8, 2013

Merge pull request #839 from richdougherty/netty-reload-839
Netty pipeline needs an executionHandler

@huntc huntc merged commit 046bc8a into playframework:master Apr 8, 2013

@richdougherty richdougherty deleted the richdougherty:netty-reload-839 branch Apr 10, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment