Issue 36643002: Don't close existing connection on HttpServer close (and stream cancel).

Issue 36643002: Don't close existing connection on HttpServer close (and stream cancel). (Closed)

Created:
7 years, 2 months ago by Anders Johnsen

Modified:
7 years, 2 months ago

Reviewers:
Søren Gjesse

CC:
reviews_dartlang.org

Base URL:
https://dart.googlecode.com/svn/branches/bleeding_edge/dart

Visibility:
Public.

More Reviews

Description

Don't close existing connection on HttpServer close (and stream cancel). This also fixes several issues exposed by this. BUG=http://code.google.com/p/dart/issues/detail?id=14317 R=sgjesse@google.com Committed: https://code.google.com/p/dart/source/detail?r=29083

Patch Set 1 #

Total comments: 10

Patch Set 2 : Fix status-file and error. #

Patch Set 3 : Add force flag and tests. #

Patch Set 4 : Add test for no keep-alive after close. #

Patch Set 5 : Always close idle connections. #

Total comments: 4

Patch Set 6 : Fix 'disable keep-alive' on server close. #

Created: 7 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+84 lines, -91 lines)			Patch
M	sdk/lib/_internal/pub/pub.status	View	1	1 chunk	+0 lines, -5 lines	0 comments	Download
M	sdk/lib/io/http.dart	View	1 2	1 chunk	+4 lines, -2 lines	0 comments	Download
M	sdk/lib/io/http_impl.dart	View	1 2 3 4 5	8 chunks	+29 lines, -18 lines	0 comments	Download
M	tests/standalone/io/http_client_connect_test.dart	View	1 2 3 4 5	4 chunks	+42 lines, -5 lines	0 comments	Download
M	tests/standalone/io/http_client_request_test.dart	View		2 chunks	+7 lines, -11 lines	0 comments	Download
M	tests/standalone/io/http_detach_socket_test.dart	View	1 2 3 4 5	1 chunk	+2 lines, -2 lines	0 comments	Download
D	tests/standalone/io/http_server_early_server_close_test.dart	View		1 chunk	+0 lines, -48 lines	0 comments	Download

Messages

Total messages: 8 (0 generated)

Expand Messages | Collapse Messages

Søren Gjesse

We need some kind for forced shutdown. https://codereview.chromium.org/36643002/diff/1/sdk/lib/io/http_impl.dart File sdk/lib/io/http_impl.dart (right): https://codereview.chromium.org/36643002/diff/1/sdk/lib/io/http_impl.dart#newcode625 sdk/lib/io/http_impl.dart:625: onError: (e) ...

7 years, 2 months ago (2013-10-23 08:14:05 UTC) #2

Anders Johnsen

I agree with an option about force shutdown, see inline. https://codereview.chromium.org/36643002/diff/1/sdk/lib/io/http_impl.dart File sdk/lib/io/http_impl.dart (right): https://codereview.chromium.org/36643002/diff/1/sdk/lib/io/http_impl.dart#newcode625 ...

7 years, 2 months ago (2013-10-23 10:23:24 UTC) #3

I agree with an option about force shutdown, see inline.

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

https://codereview.chromium.org/36643002/diff/1/sdk/lib/io/http_impl.dart#new...
sdk/lib/io/http_impl.dart:625: onError: (e) => _controller.addError(e),
On 2013/10/23 08:14:05, Søren Gjesse wrote:
> Do we want to propagate the stack trace here?

Yes, nice catch! :)

https://codereview.chromium.org/36643002/diff/1/sdk/lib/io/http_impl.dart#new...
sdk/lib/io/http_impl.dart:2009: Future close() {
On 2013/10/23 08:14:05, Søren Gjesse wrote:
> I think we still need the public close to do a full shutdown closing all
client
> connections. We could add an optional boolean force argument, but as far as I
> can see that would be (almost) the same as canceling the subscription.
> 
> Currently close has a future which completes when the server socket is closed.
> There is no future for when all the active connections are all done. If (when)
> StreamSubscription.cancel gets a future when should that complete?
> 
> Alternatively we could have a destroy method which closes all active
> connections. The future from close can then complete when all active
connections
> are terminated.
> 

The 'problem' is that there is no binding between a server-socket and the
accepted sockets. Even if the server-socket is closed, the accepted sockets will
live on. The question is then, what should a HTTP server do with the live
requests. What we did before was to destroy them, not caring about what they
were doing. That's a very aggressive approach, as the user may get broken data,
depending on when it's closed.

Moving this into a 'destroy' method makes sense. However, as it's exactly the
same semantics, I would suggest we do as with HttpClient and add a `force` flag,
with `false` as default. I think users explicit should express when they are
okay with breaking requests. What do you think?

https://codereview.chromium.org/36643002/diff/1/tests/standalone/io/http_clie...
File tests/standalone/io/http_client_connect_test.dart (left):

https://codereview.chromium.org/36643002/diff/1/tests/standalone/io/http_clie...
tests/standalone/io/http_client_connect_test.dart:84: void
testGetDataServerClose() {
On 2013/10/23 08:14:05, Søren Gjesse wrote:
> This test is removed because there is no forceful server shutdown. Adding that
> back should make it possible to keep this test.

Agreed. Let me know what you think about the `force` flag.

https://codereview.chromium.org/36643002/diff/1/tests/standalone/io/http_serv...
File tests/standalone/io/http_server_early_server_close_test.dart (left):

https://codereview.chromium.org/36643002/diff/1/tests/standalone/io/http_serv...
tests/standalone/io/http_server_early_server_close_test.dart:1: // Copyright (c)
2012, the Dart project authors.  Please see the AUTHORS file
On 2013/10/23 08:14:05, Søren Gjesse wrote:
> With forceful shutdown we can keep this test.

It's an exact duplicate of the other test (just more verbose), so I think this
should go away no matter what.

Søren Gjesse

https://codereview.chromium.org/36643002/diff/1/sdk/lib/io/http_impl.dart File sdk/lib/io/http_impl.dart (right): https://codereview.chromium.org/36643002/diff/1/sdk/lib/io/http_impl.dart#newcode2009 sdk/lib/io/http_impl.dart:2009: Future close() { On 2013/10/23 10:23:24, Anders Johnsen wrote: ...

7 years, 2 months ago (2013-10-23 11:05:27 UTC) #4

Anders Johnsen

Added `force` option. PTAL :) https://codereview.chromium.org/36643002/diff/1/sdk/lib/io/http_impl.dart File sdk/lib/io/http_impl.dart (right): https://codereview.chromium.org/36643002/diff/1/sdk/lib/io/http_impl.dart#newcode2009 sdk/lib/io/http_impl.dart:2009: Future close() { On ...

7 years, 2 months ago (2013-10-23 12:06:54 UTC) #5

Added `force` option. PTAL :)

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

https://codereview.chromium.org/36643002/diff/1/sdk/lib/io/http_impl.dart#new...
sdk/lib/io/http_impl.dart:2009: Future close() {
On 2013/10/23 11:05:28, Søren Gjesse wrote:
> On 2013/10/23 10:23:24, Anders Johnsen wrote:
> > On 2013/10/23 08:14:05, Søren Gjesse wrote:
> > > I think we still need the public close to do a full shutdown closing all
> > client
> > > connections. We could add an optional boolean force argument, but as far
as
> I
> > > can see that would be (almost) the same as canceling the subscription.
> > > 
> > > Currently close has a future which completes when the server socket is
> closed.
> > > There is no future for when all the active connections are all done. If
> (when)
> > > StreamSubscription.cancel gets a future when should that complete?
> > > 
> > > Alternatively we could have a destroy method which closes all active
> > > connections. The future from close can then complete when all active
> > connections
> > > are terminated.
> > > 
> > 
> > The 'problem' is that there is no binding between a server-socket and the
> > accepted sockets. Even if the server-socket is closed, the accepted sockets
> will
> > live on. The question is then, what should a HTTP server do with the live
> > requests. What we did before was to destroy them, not caring about what they
> > were doing. That's a very aggressive approach, as the user may get broken
> data,
> > depending on when it's closed.
> > 
> > Moving this into a 'destroy' method makes sense. However, as it's exactly
the
> > same semantics, I would suggest we do as with HttpClient and add a `force`
> flag,
> > with `false` as default. I think users explicit should express when they are
> > okay with breaking requests. What do you think?
> 
> I am fine with adding the force flag. In case force is false should the
returned
> future then complete when all client connections are done or should we have a
> separate future for that?

The way we force-close is by calling 'socket.destroy'. This means that it's a
sync operation with nothing to wait for, so it'll just be the same future in
both cases.

Søren Gjesse

LGTM https://codereview.chromium.org/36643002/diff/160008/tests/standalone/io/http_client_connect_test.dart File tests/standalone/io/http_client_connect_test.dart (right): https://codereview.chromium.org/36643002/diff/160008/tests/standalone/io/http_client_connect_test.dart#newcode89 tests/standalone/io/http_client_connect_test.dart:89: server.listen((request) { Use server.first in the test? https://codereview.chromium.org/36643002/diff/160008/tests/standalone/io/http_detach_socket_test.dart ...

7 years, 2 months ago (2013-10-23 13:23:25 UTC) #6

Anders Johnsen

Thanks, landing. https://codereview.chromium.org/36643002/diff/160008/tests/standalone/io/http_client_connect_test.dart File tests/standalone/io/http_client_connect_test.dart (right): https://codereview.chromium.org/36643002/diff/160008/tests/standalone/io/http_client_connect_test.dart#newcode89 tests/standalone/io/http_client_connect_test.dart:89: server.listen((request) { On 2013/10/23 13:23:25, Søren Gjesse ...

7 years, 2 months ago (2013-10-23 14:12:34 UTC) #7

Anders Johnsen

7 years, 2 months ago (2013-10-23 14:18:36 UTC) #8

Message was sent while issue was closed.

Committed patchset #6 manually as r29083 (presubmit successful).

Expand Messages | Collapse Messages