Fix a number of HTTP issues

Fix a number of HTTP issues

The main purpose of this change is to ensure that the HTTP server
does not start sending the response before the full request have
been read. This ended up requiring a number of changes.

1. Handle server closing connection before full request is read

If the server closes the connection before the request body have
been read this will result in the underlying socket getting
closed by the server.

2. Support writing to output stream before full request is read

Changed the server to not start sending data before the full
request have been read. In order to still make it possible to
write to the request output stream before reading all data any
data written before will be buffered. When the request have been
fully read the buffered data will be written.

3. Handle of bad/closed connections retreived from the client connection pool

When a HTTP client connection is pulled from the pool of keep
alive connections there can be situations where this connection
has just been closed by the server. This is now mittigated by
trying to make a new connection when this happens. It is only
possible to actually create a new connection if no body has been
sent.

4. Forward request headers when redirecting

Both for manual and automatic rediretion any request headers set
on the original request are now set on the redirect request as
well.

5. Invalidate sockets in the client pool if required

Open HTTP socket connections in the client pool are now
invalidated and closed if there is any activify on them while
they are idle.

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

BUG=6984
Committed: https://code.google.com/p/dart/source/detail?r=15782

[Mads Ager (google), 2012-12-05 16:02:18]

LGTM

https://codereview.chromium.org/11453006/diff/1/sdk/lib/io/http_impl.dart
File sdk/lib/io/http_impl.dart (right):

https://codereview.chromium.org/11453006/diff/1/sdk/lib/io/http_impl.dart#new...
sdk/lib/io/http_impl.dart:820: void _releaseBuffer() {
Maybe name this something like _writeBufferedResponse to make it clear that it
writes and does not just drop the buffer?

https://codereview.chromium.org/11453006/diff/1/sdk/lib/io/http_impl.dart#new...
sdk/lib/io/http_impl.dart:961: // Buffer for data written before full response
have been processed.
have -> has

https://codereview.chromium.org/11453006/diff/1/sdk/lib/io/http_impl.dart#new...
sdk/lib/io/http_impl.dart:1174: _emptyBody = false;
Should this be 

_emptyBody = buffer.length != 0 || _emptyBody;

?

https://codereview.chromium.org/11453006/diff/1/sdk/lib/io/http_impl.dart#new...
sdk/lib/io/http_impl.dart:1180: _emptyBody = false;
Ditto

https://codereview.chromium.org/11453006/diff/1/sdk/lib/io/http_impl.dart#new...
sdk/lib/io/http_impl.dart:1559: // If the connection already have a request this
is a retry of a
have -> has

https://codereview.chromium.org/11453006/diff/1/sdk/lib/io/http_impl.dart#new...
sdk/lib/io/http_impl.dart:1724: // invalidate the connection os that it is not
reused.
os -> so

https://codereview.chromium.org/11453006/diff/1/sdk/lib/io/http_impl.dart#new...
sdk/lib/io/http_impl.dart:1732: void _markRetreived() {
_markRetrieved

https://codereview.chromium.org/11453006/diff/1/sdk/lib/io/secure_socket.dart
File sdk/lib/io/secure_socket.dart (right):

https://codereview.chromium.org/11453006/diff/1/sdk/lib/io/secure_socket.dart...
sdk/lib/io/secure_socket.dart:133: if (_inputStream != null && callback != null)
{
Heh! Thanks!

[Bob Nystrom, 2012-12-05 18:27:50]

Drive-by comments:

1. This sounds fantastic! Making HTTP more robust especially in error cases
makes it much easier for us to make pub stable and reliable.

2. The CL description makes it seem like a lot of behavior is changing, but I
see very little test code here. Were there tests of this behavior that were
failing before?

[Søren Gjesse, 2012-12-06 17:23:08]

Bob,

Regarding testing then this is already tested by existing tests. We already had
several tests doing 1. and 2. and with these changes they are still working.
Some tests then revealed 3. which I then fixed. 4. is tested by two new redirect
tests. However 5. is not tested and I will add a test for that.

https://codereview.chromium.org/11453006/diff/1/sdk/lib/io/http_impl.dart
File sdk/lib/io/http_impl.dart (right):

https://codereview.chromium.org/11453006/diff/1/sdk/lib/io/http_impl.dart#new...
sdk/lib/io/http_impl.dart:820: void _releaseBuffer() {
On 2012/12/05 16:02:18, Mads Ager wrote:
> Maybe name this something like _writeBufferedResponse to make it clear that it
> writes and does not just drop the buffer?

Done.

https://codereview.chromium.org/11453006/diff/1/sdk/lib/io/http_impl.dart#new...
sdk/lib/io/http_impl.dart:961: // Buffer for data written before full response
have been processed.
On 2012/12/05 16:02:18, Mads Ager wrote:
> have -> has

Done.

https://codereview.chromium.org/11453006/diff/1/sdk/lib/io/http_impl.dart#new...
sdk/lib/io/http_impl.dart:1174: _emptyBody = false;
On 2012/12/05 16:02:18, Mads Ager wrote:
> Should this be 
> 
> _emptyBody = buffer.length != 0 || _emptyBody;
> 
> ?

Good point, however it should be

_emptyBody = buffer.length == 0 && _emptyBody;

https://codereview.chromium.org/11453006/diff/1/sdk/lib/io/http_impl.dart#new...
sdk/lib/io/http_impl.dart:1180: _emptyBody = false;
On 2012/12/05 16:02:18, Mads Ager wrote:
> Ditto

Done.

https://codereview.chromium.org/11453006/diff/1/sdk/lib/io/http_impl.dart#new...
sdk/lib/io/http_impl.dart:1559: // If the connection already have a request this
is a retry of a
On 2012/12/05 16:02:18, Mads Ager wrote:
> have -> has

Done.

https://codereview.chromium.org/11453006/diff/1/sdk/lib/io/http_impl.dart#new...
sdk/lib/io/http_impl.dart:1724: // invalidate the connection os that it is not
reused.
On 2012/12/05 16:02:18, Mads Ager wrote:
> os -> so

Done.

https://codereview.chromium.org/11453006/diff/1/sdk/lib/io/http_impl.dart#new...
sdk/lib/io/http_impl.dart:1732: void _markRetreived() {
On 2012/12/05 16:02:18, Mads Ager wrote:
> _markRetrieved
> 

Done.

[Bob Nystrom, 2012-12-06 17:47:31]

On 2012/12/06 17:23:08, Søren Gjesse wrote:
> Bob,
> 
> Regarding testing then this is already tested by existing tests. We already
had
> several tests doing 1. and 2. and with these changes they are still working.
> Some tests then revealed 3. which I then fixed. 4. is tested by two new
redirect
> tests. However 5. is not tested and I will add a test for that.

Awesome, sounds great!

- bob