remove close_idle_connections again and fix a bug where connections w…

…ould get closed too quickly
mojolicious · Aug 2, 2017 · f9ff45e · zakame · Feb 2, 2018 · f9ff45e
1 parent 6ee05f1
commit f9ff45e
Show file tree

Hide file tree

Showing 5 changed files with 22 additions and 17 deletions.
diff --git a/Changes b/Changes
@@ -1,5 +1,10 @@
 
 7.39  2017-08-02
+  - Removed experimental close_idle_connections method from
+    Mojo::Server::Daemon.
+  - Improve Mojo::Server::Daemon to log request errors.
+  - Fixed a bug where Mojo::Server::Daemon would close connections too quickly
+    and interrupt requests.
 
 7.38  2017-07-30
   - Added extra attribute to Mojolicious::Static. (jabberwok)

diff --git a/lib/Mojo/Server/Daemon.pm b/lib/Mojo/Server/Daemon.pm
@@ -25,13 +25,6 @@ sub DESTROY {
   $loop->remove($_) for keys %{$self->{connections} || {}}, @{$self->acceptors};
 }
 
-sub close_idle_connections {
-  my $self = shift;
-  my $c    = $self->{connections};
-  my $loop = $self->ioloop;
-  !$c->{$_}{tx} and $c->{$_}{requests} and $loop->remove($_) for keys %$c;
-}
-
 sub ports {
   [map { $_[0]->ioloop->acceptor($_)->port } @{$_[0]->acceptors}];
 }
@@ -96,8 +89,11 @@ sub _build_tx {
     request => sub {
       my $tx = shift;
 
+      my $req = $tx->req;
+      if (my $error = $req->error) { $self->_debug($id, $error->{message}) }
+
       # WebSocket
-      if ($tx->req->is_handshake) {
+      if ($req->is_handshake) {
         my $ws = $self->{connections}{$id}{next}
           = Mojo::Transaction::WebSocket->new(handshake => $tx);
         $self->emit(request => server_handshake $ws);
@@ -109,7 +105,8 @@ sub _build_tx {
       # Last keep-alive request or corrupted connection
       my $c = $self->{connections}{$id};
       $tx->res->headers->connection('close')
-        if $c->{requests} >= $self->max_requests || $tx->req->error;
+        and ++Mojo::IOLoop->stream($id)->{closed}
+        if $c->{requests} >= $self->max_requests || $req->error;
 
       $tx->on(resume => sub { $self->_write($id) });
       $self->_write($id);
@@ -480,13 +477,6 @@ Disable console messages.
 L<Mojo::Server::Daemon> inherits all methods from L<Mojo::Server> and
 implements the following new ones.
 
-=head2 close_idle_connections
-
-  $daemon->close_idle_connections;
-
-Close all connections without active requests. Note that this method is
-EXPERIMENTAL and might change without warning!
-
 =head2 ports
 
   my $ports = $daemon->ports;

diff --git a/lib/Mojo/Server/Prefork.pm b/lib/Mojo/Server/Prefork.pm
@@ -155,7 +155,7 @@ sub _spawn {
   # Clean worker environment
   $SIG{$_} = 'DEFAULT' for qw(CHLD INT TERM TTIN TTOU);
   $SIG{QUIT} = sub { $loop->stop_gracefully };
-  $loop->on(finish => sub { $self->max_requests(1)->close_idle_connections });
+  $loop->on(finish => sub { $self->max_requests(1) });
   delete $self->{reader};
   srand;
 

diff --git a/t/mojo/hypnotoad.t b/t/mojo/hypnotoad.t
@@ -211,6 +211,12 @@ ok !$tx->kept_alive, 'connection was not kept alive';
 is $tx->res->code, 200, 'right status';
 is $tx->res->body, 'Graceful shutdown!', 'right content';
 
+# One uncertain request that may or may not be served by the old worker
+$tx = $ua->get("http://127.0.0.1:$port1/hello");
+is $tx->res->code, 200, 'right status';
+$tx = $ua->get("http://127.0.0.1:$port2/hello");
+is $tx->res->code, 200, 'right status';
+
 # Application has been reloaded
 $tx = $ua->get("http://127.0.0.1:$port1/hello");
 ok $tx->is_finished, 'transaction is finished';

diff --git a/t/mojolicious/lite_app.t b/t/mojolicious/lite_app.t
@@ -747,10 +747,14 @@ $t->app->log->unsubscribe(message => $cb);
 # With body and max message size
 {
   local $ENV{MOJO_MAX_MESSAGE_SIZE} = 1024;
+  $log = '';
+  $cb = $t->app->log->on(message => sub { $log .= pop });
   $t->get_ok('/', '1234' x 1024)->status_is(200)
     ->header_is(Connection => 'close')
     ->content_is("Maximum message size exceeded\n"
       . "/root.html\n/root.html\n/root.html\n/root.html\n/root.html\n");
+  like $log, qr/Maximum message size exceeded/, 'right message';
+  $t->app->log->unsubscribe(message => $cb);
 }
 
 # Relaxed placeholder