Add error handling to the HTTP library

Add error handling to the HTTP library

The socket errors and HTTP parser errors are now handled by the
HTTP library and propagated to the related onError callback.

For HttpClient there is no onError on the HttpClient object
itself. Errors on active connections (including connection
errors) are reported on the HttpClientConnection. Errors on
connections sitting in the keep alive pool are handled by just
closing the connection and removing it from the pool. Errors on
the HttpClientConnection are also reported through the onError
callbacks on any active streams.

I am not particulair happy of the way errors are propagated to
the active streams but that can be refactored later.

Added more checks and tests to the HTTP parser.

R=ager@google.com, ajohnsen@google.com

BUG=
TEST=

Committed: https://code.google.com/p/dart/source/detail?r=5768

[Mads Ager (google), 2012-03-22 14:30:08]

LGTM

https://chromiumcodereview.appspot.com/9834008/diff/1/runtime/bin/http_impl.dart
File runtime/bin/http_impl.dart (right):

https://chromiumcodereview.appspot.com/9834008/diff/1/runtime/bin/http_impl.d...
runtime/bin/http_impl.dart:530: _subOnError(e);
You called this something like _propagateError in a previous change. Use that
here as well? I liked that name better. :)

https://chromiumcodereview.appspot.com/9834008/diff/1/runtime/bin/http_impl.d...
runtime/bin/http_impl.dart:1123: socket.onConnect = () {
I think it would read better if you move the onError assignment before the
onConnect assignment?

Then the comments would be:

// Until the connection is established ...
onError = handleError
// When the connection is established ...
onConnect = handleConnect

Not really important though.

https://chromiumcodereview.appspot.com/9834008/diff/1/runtime/bin/http_impl.d...
runtime/bin/http_impl.dart:1124: // Clear the error callback as it will now be
handled by the
When the connection is established, clear the ...

https://chromiumcodereview.appspot.com/9834008/diff/1/runtime/bin/http_parser...
File runtime/bin/http_parser.dart (right):

https://chromiumcodereview.appspot.com/9834008/diff/1/runtime/bin/http_parser...
runtime/bin/http_parser.dart:391: if (_remainingContent == null || dataAvailable
<= _remainingContent) {
Long line?

https://chromiumcodereview.appspot.com/9834008/diff/1/runtime/bin/http_parser...
runtime/bin/http_parser.dart:450: // Report the error through the error callback
if any. Otherwise
Maybe this should really be our behavior for all of the error handlers that we
have on anything. At this point we silently ignore most errors if no handler is
set. Maybe we should update that to throw the exception. Silent errors are bad
and really hard to track down.

We shouldn't make that part of this change though. :)

https://chromiumcodereview.appspot.com/9834008/diff/1/tests/standalone/src/io...
File tests/standalone/src/io/HttpParserTest.dart (right):

https://chromiumcodereview.appspot.com/9834008/diff/1/tests/standalone/src/io...
tests/standalone/src/io/HttpParserTest.dart:108: void checkExpectations() {
Not sure that this method adds much. Just inline?

https://chromiumcodereview.appspot.com/9834008/diff/1/tests/standalone/src/io...
tests/standalone/src/io/HttpParserTest.dart:231: void checkExpectations() {
Ditto.

[Søren Gjesse, 2012-03-23 07:34:20]

http://codereview.chromium.org/9834008/diff/1/runtime/bin/http_impl.dart
File runtime/bin/http_impl.dart (right):

http://codereview.chromium.org/9834008/diff/1/runtime/bin/http_impl.dart#newc...
runtime/bin/http_impl.dart:530: _subOnError(e);
On 2012/03/22 14:30:08, Mads Ager wrote:
> You called this something like _propagateError in a previous change. Use that
> here as well? I liked that name better. :)

Done. Sorry for the strange name.

http://codereview.chromium.org/9834008/diff/1/runtime/bin/http_impl.dart#newc...
runtime/bin/http_impl.dart:1123: socket.onConnect = () {
On 2012/03/22 14:30:08, Mads Ager wrote:
> I think it would read better if you move the onError assignment before the
> onConnect assignment?
> 
> Then the comments would be:
> 
> // Until the connection is established ...
> onError = handleError
> // When the connection is established ...
> onConnect = handleConnect
> 
> Not really important though.

Done.

http://codereview.chromium.org/9834008/diff/1/runtime/bin/http_impl.dart#newc...
runtime/bin/http_impl.dart:1124: // Clear the error callback as it will now be
handled by the
On 2012/03/22 14:30:08, Mads Ager wrote:
> When the connection is established, clear the ...

Done.

http://codereview.chromium.org/9834008/diff/1/runtime/bin/http_parser.dart
File runtime/bin/http_parser.dart (right):

http://codereview.chromium.org/9834008/diff/1/runtime/bin/http_parser.dart#ne...
runtime/bin/http_parser.dart:391: if (_remainingContent == null || dataAvailable
<= _remainingContent) {
On 2012/03/22 14:30:08, Mads Ager wrote:
> Long line?

Done.

http://codereview.chromium.org/9834008/diff/1/runtime/bin/http_parser.dart#ne...
runtime/bin/http_parser.dart:450: // Report the error through the error callback
if any. Otherwise
On 2012/03/22 14:30:08, Mads Ager wrote:
> Maybe this should really be our behavior for all of the error handlers that we
> have on anything. At this point we silently ignore most errors if no handler
is
> set. Maybe we should update that to throw the exception. Silent errors are bad
> and really hard to track down.
> 
> We shouldn't make that part of this change though. :)

That might be a good idea. This can lead to VM terminations due to uncaught
exceptions, but thats probably better than just failing silently.

http://codereview.chromium.org/9834008/diff/1/tests/standalone/src/io/HttpPar...
File tests/standalone/src/io/HttpParserTest.dart (right):

http://codereview.chromium.org/9834008/diff/1/tests/standalone/src/io/HttpPar...
tests/standalone/src/io/HttpParserTest.dart:108: void checkExpectations() {
On 2012/03/22 14:30:08, Mads Ager wrote:
> Not sure that this method adds much. Just inline?

Done.

http://codereview.chromium.org/9834008/diff/1/tests/standalone/src/io/HttpPar...
tests/standalone/src/io/HttpParserTest.dart:231: void checkExpectations() {
On 2012/03/22 14:30:08, Mads Ager wrote:
> Ditto.

Done.