Don't write to servlet output stream in a trampoline context

[jroper]

As far as I can see, this write:

https://github.com/dlecan/play2-war-plugin/blob/develop/project-code/core/common/src/main/scala/play/core/server/servlet/RequestHandler.scala#L209

and this write:

https://github.com/dlecan/play2-war-plugin/blob/develop/project-code/core/common/src/main/scala/play/core/server/servlet/RequestHandler.scala#L260

Writes to the servlet output stream in a trampoline context, with no consideration of what thread pool the current thread came from. This is incredibly dangerous, since it's a blocking call, and you have no idea which thread pool you're blocking. A single slow reading client could lock up the entire application, preventing it from serving any more requests until that slow client has finished reading everything you sent to it.

Both of these calls should be dispatched to a thread pool, and not the default Play thread pool either, but rather a thread pool that is created specifically for doing blocking IO. That thread pool should be configured by default to have at least 100 threads (the number of threads in that thread pool will be equal to the number of clients can concurrently be downloading responses).

[yanns]

@jroper thanks for the code review! I was wondering myself if the current code was right. You've anticipated my questions. ;)

[dlecan]

A single slow reading client could lock up the entire application,

Several issues have already been reported by users about "application hangs". They might be related to this excellent code review. Thank you!

[jroper]

I didn't do an overly comprehensive review here. It's important to understand exactly what calls in the servlet API can block (some are surprising), given that you're probably more familiar with the code than I am it would also be good if you did a careful review.

For the servlet 2.x implementation (I don't know how interested you are in making that as good as it could be), the best thing to do would actually be to write an iteratee that you could imperatively pull from, then you could block on the servlet containers thread to do output. That won't work of course in servlet 3.x with asynchronous request handling.

I'll be happy to review the code that fixes this.

Also, I had a look at handling the input stream, it looks like the entire stream is read into memory on the servlet containers thread, so that's fine (excluding that it means you can't stream request bodies or consume very large request bodies).

I'm interested for the chunking/dechunking, do all servlet containers double chunk if the servlet tries to do the chunking itself, or was it just some?

[yanns]

2014-02-19 23:58 GMT+01:00 James Roper notifications@github.com:

Also, I had a look at handling the input stream, it looks like the entire stream is read into memory on the servlet containers thread, so that's fine (excluding that it means you can't stream request bodies or consume very large request bodies).

I am personally not happy with consuming the whole body in memory. I was thinking chunking the body myself and pushing them into the body parser. That would mean for I must block while waiting for the body parser to reach its next state. To avoid too much blocking, the size of the chunks could be high (20 MB?)

But I am much more interested in getting the async IO with servlet 3.1 before, although it is not so easy, as I am finding bugs in containers. The specification is not precise enough in this area.

I'm interested for the chunking/dechunking, do all servlet containers double chunk if the servlet tries to do the chunking itself, or was it just some?

As far as I remember, yes. The following test showed an unexpected response size, as the servlet containers were chunking the already chunked responses.

[dlecan]

We used to have issues with reading inputstreams thanks to enumerators (request hangs). Enumerator.fromStream(is).andThen(Enumerator.eof) for eg. Reading the stream in the memory was a way to solve theses issues.

169 was a proposal to improve this, but was not integrated into main branch.

[yanns]

After having wrote the asynchronous output for servlet 3.1: https://github.com/yanns/play2-war-plugin/commit/b580b8c9000526aff2d1c6ab07443123fe109177, I am now sure that the pushPlayResultToServletOS method is called from the servlet thread.

This method uses only the trampoline context. If I understand correctly, it means that all callbacks will be run with the same thread, the servlet thread.

The writing to the servlet output stream should also be done with the servlet thread. @jroper can you confirm or correct me?

[yanns]

After having discussed this with @jroper and displaying the thread name in the logs, I can confirm that this issue is still present.

The blocking writing to the servlet output stream is currently done with the play-akka.actor.default-dispatcher.

[yanns]

To solve this issue, I see 3 possibilities:

1. using the servlet IO Thread. I do not really see how it can be possible.
2. using a special thread pool for all blocking IOs, configured to use 100 to 300 threads for example. It implies that the application will use much more threads as a "normal" war application.
3. documenting to configure the default thread pool as in the chapter "Highly synchronous" in http://www.playframework.com/documentation/2.3.x/ThreadPools

@jroper I'd like to have your opinion on this point.