playframework/Play20 — pull request #996 by richdougherty (2013-04-11 09:47:19+00:00)
Title: Allow user code supplied to iteratees and enumerators to run in an ExecutionContext (#807)

This is the first part of #807.

This PR updates the iteratee library so that user code can be run in its own `ExecutionContext`. Many methods have been updated to accept an `ExecutionContext` argument. For example, `map` now accepts an `ExecutionContext`, and the function passed to `map` will run in that context. The ability to run code in a different context will be used by the Play framework to isolate the execution of user code from internal Play code.

All built-in iteratee code runs in the default Scala `ExecutionContext` (`ExecutionContext.global`). Only user code runs in the `ExecutionContext`s passed in as arguments.

Before merging this PR I additionally plan to:
- [ ] Update scaladoc.
- [ ] Write tests for iteratee package methods I've changed that currently lack tests.
(I'll do this while the PR is being reviewed.)

But please review in the meantime!

#### Code removed

I removed some deprecated code, to reduce the work of updating the iteratee module.

- Removed deprecated `Concurrent.hub` and associated `Hub` trait.
- Removed deprecated `Enumerator.imperative` and `Enumerator.pushee`.
- Removed deprecated `Enumerator.callback` method.
- Removed deprecated `PushEnumerator` class.

More controversially, I also removed some code that *wasn't* deprecated. I did this when the code had a strong relationship to the deprecated code and I felt it may have been mistakenly left un-deprecated. The code I removed wasn't used anywhere else in the Play framework and also had errors or other issues.

We may want to discuss this. :) I'll understand if any of the code I removed needs to be reinstated.

- Removed *non-deprecated* `Enumerator.callback1` method (related to deprecated `callback` method; unused; untested; `onError` argument not called).
- Removed *non-deprecated* `Concurrent.broadcast` method (basically wraps deprecated `hub` method; unused; untested).

#### Next PR

I haven't completed #807 with this PR, more work will be required.

First we'll want to update the Play framework to use this code.
- Work out how to structure threadpools in Iteratees and in Play for both internal and user code.
- Change code outside Iteratee package to run user code in the correct `ExecutionContext`, e.g. user actions, filters, etc.

Then we'll want to finish off the iteratee module changes to close the issue.
- Audit Iteratee package to make sure all user code runs in a user-supplied `ExecutionContext`, e.g. will need to update `joinI`.
- Add tests for these methods if needed.
- Make all `ExecutionContext` params and values implicit, once we're sure everything is wired together properly.
- Once `ExecutionContext`s are implicit, remove any now-unneeded explicit `ExecutionContext`s.

There are a few other changes we could consider too:
- Make `Concurrent.patchPanel`'s `isClosed` var `@volatile` annotation? Need to get commented-out test working first.
- Make an `ExecutionContext` that executes immediately in the same thread, to use for in certain cases, e.g. the simple nop function in `Iteratee.ignore.`
- Perf test before and after to identify impact of changes.