Close connections on server.close(), and call onError on early connection close.

Close connections on server.close(), and call onError on early connection close.
Committed: https://code.google.com/p/dart/source/detail?r=5452

[Anders Johnsen, 2012-03-14 09:20:05]

Added testcase.

[Søren Gjesse, 2012-03-14 09:26:59]

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

https://chromiumcodereview.appspot.com/9700012/diff/1/runtime/bin/http_impl.d...
runtime/bin/http_impl.dart:946: void _onDisconnected() {
We probably need to handle this slightly differently. If the response has either
a Content-Length or chuncked encoding then this should be fine. However in the
case where neither Content-Length nor chunked transfer is specified then closing
the connection is the correct way of indicating the end of the response (see RFC
2616 section "4.4 Message Length").

To actually test this with the Dart HTTP server we need to add some way of
sending a response without Content-Length and also not use chunked encoding.

https://chromiumcodereview.appspot.com/9700012/diff/1/runtime/bin/http_impl.d...
runtime/bin/http_impl.dart:949: new HttpException("Client disconnected before
response was sent."));
"was sent" should probably be "was received".

https://chromiumcodereview.appspot.com/9700012/diff/2001/tests/standalone/src...
File tests/standalone/src/io/HttpServerEarlyCloseTest.dart (right):

https://chromiumcodereview.appspot.com/9700012/diff/2001/tests/standalone/src...
tests/standalone/src/io/HttpServerEarlyCloseTest.dart:36: main() {
2 space indent.

[Anders Johnsen, 2012-03-14 09:34:43]

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

https://chromiumcodereview.appspot.com/9700012/diff/1/runtime/bin/http_impl.d...
runtime/bin/http_impl.dart:946: void _onDisconnected() {
On 2012/03/14 09:26:59, Søren Gjesse wrote:
> We probably need to handle this slightly differently. If the response has
either
> a Content-Length or chuncked encoding then this should be fine. However in the
> case where neither Content-Length nor chunked transfer is specified then
closing
> the connection is the correct way of indicating the end of the response (see
RFC
> 2616 section "4.4 Message Length").
> 
> To actually test this with the Dart HTTP server we need to add some way of
> sending a response without Content-Length and also not use chunked encoding.

My initial understanding was that _onDataEnd was called iff all content have
been received, independent of how the data-end was marked. How can we go about
adding such an event?

https://chromiumcodereview.appspot.com/9700012/diff/1/runtime/bin/http_impl.d...
runtime/bin/http_impl.dart:949: new HttpException("Client disconnected before
response was sent."));
On 2012/03/14 09:26:59, Søren Gjesse wrote:
> "was sent" should probably be "was received".

Done.

https://chromiumcodereview.appspot.com/9700012/diff/2001/tests/standalone/src...
File tests/standalone/src/io/HttpServerEarlyCloseTest.dart (right):

https://chromiumcodereview.appspot.com/9700012/diff/2001/tests/standalone/src...
tests/standalone/src/io/HttpServerEarlyCloseTest.dart:36: main() {
On 2012/03/14 09:27:00, Søren Gjesse wrote:
> 2 space indent.

Done.

[Søren Gjesse, 2012-03-14 10:03:29]

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

https://chromiumcodereview.appspot.com/9700012/diff/1/runtime/bin/http_impl.d...
runtime/bin/http_impl.dart:946: void _onDisconnected() {
On 2012/03/14 09:34:43, ajohnsen wrote:
> On 2012/03/14 09:26:59, Søren Gjesse wrote:
> > We probably need to handle this slightly differently. If the response has
> either
> > a Content-Length or chuncked encoding then this should be fine. However in
the
> > case where neither Content-Length nor chunked transfer is specified then
> closing
> > the connection is the correct way of indicating the end of the response (see
> RFC
> > 2616 section "4.4 Message Length").
> > 
> > To actually test this with the Dart HTTP server we need to add some way of
> > sending a response without Content-Length and also not use chunked encoding.
> 
> My initial understanding was that _onDataEnd was called iff all content have
> been received, independent of how the data-end was marked. How can we go about
> adding such an event?

You are right this should be fixed in the HTTP parser. The HTTP parser should
have an additional method connectionClosed. Here The HTTP parser needs to handle
closing sockets correctly. It should emit a dataEnd event if we are in the case
where neither Content_Length nor chunked encoding is specified, and receiving
close is fine and otherwise emmit an onError event (currently missing)
indicating premature disconnect.

[Anders Johnsen, 2012-03-14 10:08:53]

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

https://chromiumcodereview.appspot.com/9700012/diff/1/runtime/bin/http_impl.d...
runtime/bin/http_impl.dart:946: void _onDisconnected() {
On 2012/03/14 10:03:30, Søren Gjesse wrote:
> On 2012/03/14 09:34:43, ajohnsen wrote:
> > On 2012/03/14 09:26:59, Søren Gjesse wrote:
> > > We probably need to handle this slightly differently. If the response has
> > either
> > > a Content-Length or chuncked encoding then this should be fine. However in
> the
> > > case where neither Content-Length nor chunked transfer is specified then
> > closing
> > > the connection is the correct way of indicating the end of the response
(see
> > RFC
> > > 2616 section "4.4 Message Length").
> > > 
> > > To actually test this with the Dart HTTP server we need to add some way of
> > > sending a response without Content-Length and also not use chunked
encoding.
> > 
> > My initial understanding was that _onDataEnd was called iff all content have
> > been received, independent of how the data-end was marked. How can we go
about
> > adding such an event?
> 
> You are right this should be fixed in the HTTP parser. The HTTP parser should
> have an additional method connectionClosed. Here The HTTP parser needs to
handle
> closing sockets correctly. It should emit a dataEnd event if we are in the
case
> where neither Content_Length nor chunked encoding is specified, and receiving
> close is fine and otherwise emmit an onError event (currently missing)
> indicating premature disconnect.

Want me to make it a part of this cl, or is it something we want to visit
later(adding TODO)?

[Søren Gjesse, 2012-03-14 10:12:06]

lgtm

If you don't want to do it now just commit this and file a bug - things are not
getting worse.

[Anders Johnsen, 2012-03-14 11:25:16]

Committed! :)