fix bug in Mojo::IOLoop where too many connections could be accepted

mojolicious · Mar 2, 2016 · 8c39355 · 8c39355
1 parent e32c80b
commit 8c39355
Show file tree

Hide file tree

Showing 10 changed files with 37 additions and 34 deletions.
diff --git a/Changes b/Changes
@@ -1,7 +1,9 @@
 
 6.52  2016-03-02
   - Added is_accepting method to Mojo::IOLoop::Server.
+  - Added -M option to daemon command.
   - Improved responsiveness of stop_gracefully method in Mojo::IOLoop.
+  - Fixed bug in Mojo::IOLoop where too many connections could be accepted.
   - Fixed a few concurrency bugs in Mojo::IOLoop.
 
 6.51  2016-02-29

diff --git a/lib/Mojo/IOLoop.pm b/lib/Mojo/IOLoop.pm
@@ -16,7 +16,7 @@ use constant DEBUG => $ENV{MOJO_IOLOOP_DEBUG} || 0;
 
 has max_accepts     => 0;
 has max_connections => 1000;
-has multi_accept    => sub { shift->max_connections > 50 ? 50 : 1 };
+has multi_accept    => sub { shift->max_connections };
 has reactor         => sub {
   my $class = Mojo::Reactor::Poll->detect;
   warn "-- Reactor initialized ($class)\n" if DEBUG;
@@ -345,8 +345,8 @@ C<1000>.
   my $multi = $loop->multi_accept;
   $loop     = $loop->multi_accept(100);
 
-Number of connections to accept at once, defaults to C<50> or C<1>, depending
-on if the value of L</"max_connections"> is smaller than C<50>.
+Number of connections to accept at once, defaults to the value of
+L</"max_connections">.
 
 =head2 reactor
 

diff --git a/lib/Mojo/IOLoop/Server.pm b/lib/Mojo/IOLoop/Server.pm
@@ -118,7 +118,7 @@ sub _accept {
 
   # Greedy accept
   for (1 .. $self->multi_accept) {
-    return unless my $handle = $self->{handle}->accept;
+    return unless $self->{active} && (my $handle = $self->{handle}->accept);
     $handle->blocking(0);
 
     # Disable Nagle's algorithm

diff --git a/lib/Mojo/Server/Daemon.pm b/lib/Mojo/Server/Daemon.pm
@@ -12,7 +12,7 @@ use Scalar::Util 'weaken';
 use constant DEBUG => $ENV{MOJO_DAEMON_DEBUG} || 0;
 
 has acceptors => sub { [] };
-has [qw(backlog max_clients silent)];
+has [qw(backlog max_clients multi_accept silent)];
 has inactivity_timeout => sub { $ENV{MOJO_INACTIVITY_TIMEOUT} // 15 };
 has ioloop => sub { Mojo::IOLoop->singleton };
 has listen => sub { [split ',', $ENV{MOJO_LISTEN} || 'http://*:3000'] };
@@ -43,6 +43,7 @@ sub start {
   # Resume accepting connections
   my $loop = $self->ioloop;
   if (my $max = $self->max_clients) { $loop->max_connections($max) }
+  if (defined(my $multi = $self->multi_accept)) { $loop->multi_accept($multi) }
   if (my $servers = $self->{servers}) {
     push @{$self->acceptors}, $loop->acceptor(delete $servers->{$_})
       for keys %$servers;
@@ -438,6 +439,14 @@ L<Mojo::IOLoop/"max_connections">.
 
 Maximum number of keep-alive requests per connection, defaults to C<25>.
 
+=head2 multi_accept
+
+  my $multi = $daemon->multi_accept;
+  $daemon   = $daemon->multi_accept(100);
+
+Number of connections to accept at once, passed along to
+L<Mojo::IOLoop/"multi_accept">.
+
 =head2 silent
 
   my $bool = $daemon->silent;

diff --git a/lib/Mojo/Server/Prefork.pm b/lib/Mojo/Server/Prefork.pm
@@ -10,9 +10,8 @@ has accepts => 1000;
 has cleanup => 1;
 has [qw(graceful_timeout heartbeat_timeout)] => 20;
 has heartbeat_interval => 5;
-has 'multi_accept';
-has pid_file => sub { catfile tmpdir, 'prefork.pid' };
-has workers => 4;
+has pid_file           => sub { catfile tmpdir, 'prefork.pid' };
+has workers            => 4;
 
 sub DESTROY {
   my $self = shift;
@@ -65,7 +64,6 @@ sub run {
 
   # Prepare event loop
   my $loop = $self->ioloop->max_accepts($self->accepts);
-  if (defined(my $multi = $self->multi_accept)) { $loop->multi_accept($multi) }
 
   # Pipe for worker communication
   pipe($self->{reader}, $self->{writer}) or die "Can't create pipe: $!";
@@ -399,14 +397,6 @@ Heartbeat interval in seconds, defaults to C<5>.
 Maximum amount of time in seconds before a worker without a heartbeat will be
 stopped gracefully, defaults to C<20>.
 
-=head2 multi_accept
-
-  my $multi = $prefork->multi_accept;
-  $prefork  = $prefork->multi_accept(100);
-
-Number of connections to accept at once, passed along to
-L<Mojo::IOLoop/"multi_accept">.
-
 =head2 pid_file
 
   my $file = $prefork->pid_file;

diff --git a/lib/Mojolicious/Command/daemon.pm b/lib/Mojolicious/Command/daemon.pm
@@ -15,9 +15,10 @@ sub run {
     'b|backlog=i'            => sub { $daemon->backlog($_[1]) },
     'c|clients=i'            => sub { $daemon->max_clients($_[1]) },
     'i|inactivity-timeout=i' => sub { $daemon->inactivity_timeout($_[1]) },
-    'l|listen=s'   => \my @listen,
-    'p|proxy'      => sub { $daemon->reverse_proxy(1) },
-    'r|requests=i' => sub { $daemon->max_requests($_[1]) };
+    'l|listen=s'       => \my @listen,
+    'M|multi-accept=i' => sub { $daemon->multi_accept($_[1]) },
+    'p|proxy'          => sub { $daemon->reverse_proxy(1) },
+    'r|requests=i'     => sub { $daemon->max_requests($_[1]) };
 
   $daemon->listen(\@listen) if @listen;
   $daemon->run;
@@ -54,6 +55,9 @@ Mojolicious::Command::daemon - Daemon command
     -l, --listen <location>              One or more locations you want to
                                          listen on, defaults to the value of
                                          MOJO_LISTEN or "http://*:3000"
+    -M, --multi-accept <number>          Number of connections to accept at
+                                         once, defaults to the value of
+                                         --clients
     -m, --mode <name>                    Operating mode for your application,
                                          defaults to the value of
                                          MOJO_MODE/PLACK_ENV or "development"

diff --git a/lib/Mojolicious/Command/prefork.pm b/lib/Mojolicious/Command/prefork.pm
@@ -68,7 +68,8 @@ Mojolicious::Command::prefork - Prefork command
                                          listen on, defaults to the value of
                                          MOJO_LISTEN or "http://*:3000"
     -M, --multi-accept <number>          Number of connections to accept at
-                                         once, defaults to 50
+                                         once, defaults to the value of
+                                         --clients
     -m, --mode <name>                    Operating mode for your application,
                                          defaults to the value of
                                          MOJO_MODE/PLACK_ENV or "development"

diff --git a/t/mojo/daemon.t b/t/mojo/daemon.t
@@ -261,14 +261,16 @@ like $buffer, qr/Mojo$/, 'transactions were pipelined';
 
 # Throttling
 $daemon = Mojo::Server::Daemon->new(
-  app         => $app,
-  listen      => ['http://127.0.0.1'],
-  max_clients => 23,
-  silent      => 1
+  app          => $app,
+  listen       => ['http://127.0.0.1'],
+  max_clients  => 23,
+  multi_accept => 3,
+  silent       => 1
 );
 is scalar @{$daemon->acceptors}, 0, 'no active acceptors';
 is scalar @{$daemon->start->start->acceptors}, 1, 'one active acceptor';
 is $daemon->ioloop->max_connections, 23, 'right value';
+is $daemon->ioloop->multi_accept,    3,  'right value';
 $id = $daemon->acceptors->[0];
 ok !!Mojo::IOLoop->acceptor($id), 'acceptor has been added';
 is scalar @{$daemon->stop->acceptors}, 0, 'no active acceptors';

diff --git a/t/mojo/ioloop.t b/t/mojo/ioloop.t
@@ -26,13 +26,10 @@ is ref $loop->reactor, 'MyReactor', 'right class';
 # Defaults
 $loop = Mojo::IOLoop->new;
 is $loop->max_connections, 1000, 'right default';
-is $loop->multi_accept,    50,   'right default';
-$loop = Mojo::IOLoop->new(max_connections => 51);
-is $loop->max_connections, 51, 'right value';
-is $loop->multi_accept,    50, 'right value';
+is $loop->multi_accept,    1000, 'right default';
 $loop = Mojo::IOLoop->new(max_connections => 10);
 is $loop->max_connections, 10, 'right value';
-is $loop->multi_accept,    1,  'right value';
+is $loop->multi_accept,    10, 'right value';
 $loop = Mojo::IOLoop->new(multi_accept => 10);
 is $loop->max_connections, 1000, 'right value';
 is $loop->multi_accept,    10,   'right value';
@@ -293,7 +290,7 @@ is $loop->max_accepts, 1, 'right value';
 
 # Connection limit
 $err  = '';
-$loop = Mojo::IOLoop->new->max_connections(2);
+$loop = Mojo::IOLoop->new->max_connections(2)->multi_accept(1);
 my @accepting;
 $id = $loop->server(
   {address => '127.0.0.1'} => sub {

diff --git a/t/mojo/prefork.t b/t/mojo/prefork.t
@@ -100,7 +100,6 @@ $prefork = Mojo::Server::Prefork->new(
   accepts            => 500,
   heartbeat_interval => 0.5,
   listen             => ["http://*:$port"],
-  multi_accept       => 3,
   workers            => 1
 );
 $prefork->unsubscribe('request');
@@ -123,8 +122,7 @@ $prefork->once(
 $prefork->on(reap => sub { push @reap, pop });
 $prefork->on(finish => sub { $graceful = pop });
 $prefork->run;
-is $prefork->ioloop->max_accepts,  500, 'right value';
-is $prefork->ioloop->multi_accept, 3,   'right value';
+is $prefork->ioloop->max_accepts, 500, 'right value';
 is scalar @spawn, 1, 'one worker spawned';
 is scalar @reap,  1, 'one worker reaped';
 ok !$graceful, 'server has been stopped immediately';