fixed bug where Mojo::UserAgent would keep too many connections alive (…

…closes #644, closes #645)
mojolicious · Jul 12, 2014 · c31bc24 · c31bc24
1 parent 94e5fe6
commit c31bc24
Show file tree

Hide file tree

Showing 4 changed files with 28 additions and 9 deletions.
diff --git a/Changes b/Changes
@@ -3,6 +3,7 @@
   - Added json_like, json_message_like, json_message_unlike and json_unlike
     methods to Test::Mojo.
   - Improved HTML5.1 compliance of Mojo::DOM::HTML.
+  - Fixed bug where Mojo::UserAgent would keep too many connections alive.
   - Fixed Mojo::Reactor::Poll bug where watchers were active after they have
     been removed. (jberger)
 

diff --git a/lib/Mojo/UserAgent.pm b/lib/Mojo/UserAgent.pm
@@ -201,17 +201,17 @@ sub _connection {
 sub _dequeue {
   my ($self, $nb, $name, $test) = @_;
 
-  my $found;
   my $loop = $self->_loop($nb);
-  my $old  = $self->{$nb ? 'nb_queue' : 'queue'} || [];
-  my $new  = $self->{$nb ? 'nb_queue' : 'queue'} = [];
+  my $old = $self->{$nb ? 'nb_queue' : 'queue'} ||= [];
+  my ($found, @new);
   for my $queued (@$old) {
-    push @$new, $queued and next if $found || !grep { $_ eq $name } @$queued;
+    push @new, $queued and next if $found || !grep { $_ eq $name } @$queued;
 
     # Search for id/name and sort out corrupted connections if necessary
     next unless my $stream = $loop->stream($queued->[1]);
     $test && $stream->is_readable ? $stream->close : ($found = $queued->[1]);
   }
+  @$old = @new;
 
   return $found;
 }
@@ -222,8 +222,8 @@ sub _enqueue {
   # Enforce connection limit
   my $queue = $self->{$nb ? 'nb_queue' : 'queue'} ||= [];
   my $max = $self->max_connections;
-  $self->_remove(shift(@$queue)->[1]) while @$queue > $max;
-  push @$queue, [$name, $id] if $max;
+  $self->_remove(shift(@$queue)->[1]) while @$queue && @$queue >= $max;
+  $max ? push(@$queue, [$name, $id]) : $self->_loop($nb)->stream($id)->close;
 }
 
 sub _error {
@@ -569,7 +569,8 @@ Local address to bind to.
   $ua     = $ua->max_connections(5);
 
 Maximum number of keep-alive connections that the user agent will retain
-before it starts closing the oldest ones, defaults to C<5>.
+before it starts closing the oldest ones, defaults to C<5>. Setting the value
+to C<0> will prevent any connections from being kept alive.
 
 =head2 max_redirects
 

diff --git a/t/mojo/user_agent.t b/t/mojo/user_agent.t
@@ -521,4 +521,19 @@ ok $tx->success, 'successful';
 is $tx->res->code, 200,   'right status';
 is $tx->res->body, 'Hi!', 'right content';
 
+# Connection limit
+$ua = Mojo::UserAgent->new(max_connections => 2);
+my $result;
+Mojo::IOLoop->delay(
+  sub {
+    my $delay = shift;
+    $ua->get('/' => $delay->begin) for 1 .. 5;
+  },
+  sub {
+    my $delay = shift;
+    $result = [grep {defined} map { Mojo::IOLoop->stream($_->connection) } @_];
+  }
+)->wait;
+is scalar @$result, 2, 'two active connections';
+
 done_testing();
diff --git a/t/mojo/user_agent_online.t b/t/mojo/user_agent_online.t
@@ -55,7 +55,9 @@ my $sock = IO::Socket::INET->new(PeerAddr => 'mojolicio.us', PeerPort => 80);
 my $address = $sock->sockhost;
 isnt $address, '127.0.0.1', 'different address';
 $ua->local_address('127.0.0.1')->max_connections(0);
-is $ua->get('/remote_address')->res->body, '127.0.0.1', 'right address';
+my $tx = $ua->get('/remote_address');
+ok !$ua->ioloop->stream($tx->connection), 'connection is not active';
+is $tx->res->body, '127.0.0.1', 'right address';
 $ua->local_address($address);
 is $ua->get('/remote_address')->res->body, $address, 'right address';
 
@@ -64,7 +66,7 @@ $ua = Mojo::UserAgent->new;
 
 # Connection refused
 my $port = Mojo::IOLoop::Server->generate_port;
-my $tx = $ua->build_tx(GET => "http://localhost:$port");
+$tx = $ua->build_tx(GET => "http://localhost:$port");
 $ua->start($tx);
 ok $tx->is_finished, 'transaction is finished';
 ok $tx->error,       'has error';