Bug fixes and buffer improvements

[slemesle]

Hi, I've done some very little enhancements on youre base code : Fix a Deft crash on cancelled chanel. Check ThreadSafety and improve MessageDigest use. Improved request buffer string conversion using CharsetDecoder.

There's also some formating.

Regards.

[rschildmeijer]

Hi slemesle,

Some comments on your pull request.

• Almost the entire IOLoop is marked as changed, even though there's only a small change (try catch added)
• The same for HttpUtil
• The explicit synchronization around md is not necessary. There's only one thread (the IOLoop thread) that is allowed to access that code
• CharsetDecoder is new to me. Have you noticed any performance impact after the switch?

[slemesle]

Hi rschildmeijer,

For HttpUtil and IOLoop, it's just an eclipse auto formatter. I apologize for this change, in fact, I did not have enough time to rework my patch sorry for that ;-). The try catch block added in IOLoop resolve a deft crash on cancelled key processing. For the CharsetDecoder, yes, I've seen real performance improvements, HTTP request parsing is just faster. Now for the MessageDigest part, I had ArrayOutOfBoundExceptions on this call and found it good first reset the MessageDigest after each call and lock it for Thread safety. In my test, I use an ExecutorService to run asynchronously the access to a MongoDB database and send the response from the ThreadPool. Since I am calling reponse.finish() from another Thread, I think the synchronized is just required in my case. In fact it solves the ArrayOutOfBoundException and MessageDigest instance is not ThreadSafe without this.

Regards,

P.S. I think the HTTP parser can be optimized using something like Apache HttpCore or parsing directly from the buffer. Have a look and tell me what you think about that.

[rschildmeijer]

Hi again,

(Regarding the auto formatter) There is a mailing list thread discussing coding standards (http://groups.google.com/group/deft-web-server/browse_thread/thread/10d2096f13c0b745). Until things have converged and settled I think we should leave the current formatting as is. (Feel free to raise your voice in that thread btw :) )

The "cancelled key processing" exception sounds interesting and could very well be bug in the current code base. Do you mind showing me an example when this occurs?

(Regarding CharsetDecoder) Nicholas Whitehead improved the http request parsing by ~8% with his patch for #104: https://github.com/rschildmeijer/deft/commit/c4bf2e0451dbfdcc87cf81022ba100bcfe3661c0 Maybe we could improve this even further using the CharsetDecoder class. Feel free do experiment on your own, benchmark, and create an issue with patch supplied if you notice any performance increase.

You said you are calling response.finish() from another thread. That is dangerous and not deterministic. (The only thread that is allowed to call the methods on a org.deftserver.web.http.HttpResponse is the io loop thread).

Before we have #103 (https://github.com/rschildmeijer/deft/issues#issue/103) (or a mongodb client lib that is compatible with deft == runs in defts ioloop) in place, its not possible to use third party asynchronous libraries. My best tips is to use a blocking mongodb client api and block the ioloop (good reading: https://github.com/facebook/tornado/wiki/Threading-and-concurrency) (Thanks once again Ben for this excellent article).

Using Apache HttpCore for request parsing is interesting. I guess that Deft's current request parsing is much more simple than the one done in HttpCore. If you don't want to do this benchmarking by yourself, feel free to create an issue and hopefully someone else will find it interesting and spend some time on it (For me its mostly a matter of time and priorities. Time that I don't have right now :) ).