Refactor the close and error handling of HTTP connections

Refactor the close and error handling of HTTP connections

* Unify the handling of connection close and connection error.
* The HttpConnection and HttpClientConnection now have separate
  close/error handling
* The HTTP parser gets the connection close indication as it might
  signal end of body for responses with no other length indication

R=ajohnsen@google.com

BUG=
TEST=

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

[Anders Johnsen, 2012-04-02 13:55:34]

First small round of comments.

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

http://codereview.chromium.org/9956062/diff/1001/runtime/bin/http_impl.dart#n...
runtime/bin/http_impl.dart:506: _onConnectionClosed(null);
This is really neat, I like how onError and onClosed are very close to
identical.

http://codereview.chromium.org/9956062/diff/1001/runtime/bin/http_impl.dart#n...
runtime/bin/http_impl.dart:551: if (e != null && onError != null) {
Do we want to signal onError before or after taking care of "closing" it? Same
goes for onClosed.

http://codereview.chromium.org/9956062/diff/1001/runtime/bin/http_impl.dart#n...
runtime/bin/http_impl.dart:558: if (onClosed != null) {
Here we can end up calling both onError and onClosed. That's okay, right?

[Søren Gjesse, 2012-04-02 14:11:33]

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

http://codereview.chromium.org/9956062/diff/1001/runtime/bin/http_impl.dart#n...
runtime/bin/http_impl.dart:551: if (e != null && onError != null) {
On 2012/04/02 13:55:35, ajohnsen wrote:
> Do we want to signal onError before or after taking care of "closing" it? Same
> goes for onClosed.

I think doing it here is fine. I moved the stream stuff op here as well to make
the rest more clear.

One thing to note is that this can be called again through
_httpParser.connectionClosed() (recursively) if the close results in a HTTP
error.

http://codereview.chromium.org/9956062/diff/1001/runtime/bin/http_impl.dart#n...
runtime/bin/http_impl.dart:558: if (onClosed != null) {
On 2012/04/02 13:55:35, ajohnsen wrote:
> Here we can end up calling both onError and onClosed. That's okay, right?

Good point. Changed so that either onError or onClosed is called as this is the
convention we normally use.

[Anders Johnsen, 2012-04-03 05:36:37]

LGTM, can't wait to try this out! :) Thank you

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

http://codereview.chromium.org/9956062/diff/9001/runtime/bin/http_impl.dart#n...
runtime/bin/http_impl.dart:652: _connections.remove(connection);
So, here the order matters. In the future when we ask for current connections,
do we want include this connection if called from onError. The problem is that
it's not valid at this point.

http://codereview.chromium.org/9956062/diff/9001/tests/standalone/src/io/Http...
File tests/standalone/src/io/HttpServerSocketTest.dart (right):

http://codereview.chromium.org/9956062/diff/9001/tests/standalone/src/io/Http...
tests/standalone/src/io/HttpServerSocketTest.dart:12: SocketMock this._socket);
Oops, ty!

http://codereview.chromium.org/9956062/diff/9001/tests/standalone/standalone....
File tests/standalone/standalone.status (left):

http://codereview.chromium.org/9956062/diff/9001/tests/standalone/standalone....
tests/standalone/standalone.status:8: io/HttpServerEarlyClientCloseTest: Skip #
To be fixed asap.
Yay! :) Thanks for doing this!

[Anders Johnsen, 2012-04-03 07:20:16]

https://chromiumcodereview.appspot.com/9956062/diff/9001/runtime/bin/http_par...
File runtime/bin/http_parser.dart (right):

https://chromiumcodereview.appspot.com/9956062/diff/9001/runtime/bin/http_par...
runtime/bin/http_parser.dart:463: error(e);
We should return after 'error(e)' so we don't hit both cases and reports two
errors.

https://chromiumcodereview.appspot.com/9956062/diff/9001/runtime/bin/http_par...
runtime/bin/http_parser.dart:481: error(e);
Maybe return here as well, just for good measure.

[Anders Johnsen, 2012-04-10 08:18:53]

LGTM, last few comments.

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

http://codereview.chromium.org/9956062/diff/13001/runtime/bin/http_impl.dart#...
runtime/bin/http_impl.dart:484: if (!_error) {
I think we should add "&& !_closing".

http://codereview.chromium.org/9956062/diff/13001/runtime/bin/http_impl.dart#...
runtime/bin/http_impl.dart:490: if (!_error) {
Same here.

http://codereview.chromium.org/9956062/diff/13001/runtime/bin/http_impl.dart#...
runtime/bin/http_impl.dart:516: void _onClosed() {
And a "_closing = true;" here.

http://codereview.chromium.org/9956062/diff/13001/runtime/bin/http_impl.dart#...
runtime/bin/http_impl.dart:592: _closing = true;
And remove it here, since we mark it in super class.

[Søren Gjesse, 2012-04-10 13:10:07]

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

http://codereview.chromium.org/9956062/diff/9001/runtime/bin/http_impl.dart#n...
runtime/bin/http_impl.dart:652: _connections.remove(connection);
On 2012/04/03 05:36:37, ajohnsen wrote:
> So, here the order matters. In the future when we ask for current connections,
> do we want include this connection if called from onError. The problem is that
> it's not valid at this point.

Moved the remove up before the _onError call. In general when asking for the
current connections you can still get connections which are not valid.

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

http://codereview.chromium.org/9956062/diff/9001/runtime/bin/http_parser.dart...
runtime/bin/http_parser.dart:463: error(e);
On 2012/04/03 07:20:16, ajohnsen wrote:
> We should return after 'error(e)' so we don't hit both cases and reports two
> errors.

Done.

http://codereview.chromium.org/9956062/diff/9001/runtime/bin/http_parser.dart...
runtime/bin/http_parser.dart:481: error(e);
On 2012/04/03 07:20:16, ajohnsen wrote:
> Maybe return here as well, just for good measure.

Done.