patches: reproxy memleak problems and client-backend becoming disconnected

[John Berthels]

Hi,

The worst memory leak problems we've been having have been fixed by
this patch from 2006 on the list (some remain, but they are much less
severe):

http://osdir.com/ml/web.server.perlbal.general/2006-10/msg00015.html

(updated patch against current svn below):

diff --git a/lib/Perlbal/BackendHTTP.pm b/lib/Perlbal/BackendHTTP.pm
index cbd7be6..725bd63 100644
--- a/lib/Perlbal/BackendHTTP.pm
+++ b/lib/Perlbal/BackendHTTP.pm
@@ -193,7 +193,10 @@ sub close {

     # tell our client that we're gone
     if (my $client = $self->{client}) {
-        $client->backend(undef);
+        # don't clobber any other backend
+        if ($client->backend() && $self == $client->backend()) {
+            $client->backend(undef);
+        }
         $self->{client} = undef;
     }

Also, ClientProxy::too_far_behind_backend always returns false if the
client has no backend. If the client does lose the backend (but the
backend still has the client), this causes the entire file being
reproxied to be dumped into memory (which was the cause of our memory
usage spikes - we're serving large files).

This patch stops that happening, so the impact if the client-backend
connections breaks is less:

diff --git a/lib/Perlbal/ClientProxy.pm b/lib/Perlbal/ClientProxy.pm
index f056ab4..2612403 100644
--- a/lib/Perlbal/ClientProxy.pm
+++ b/lib/Perlbal/ClientProxy.pm
@@ -192,7 +192,7 @@ sub try_next_uri {
 # returns true if this ClientProxy is too many bytes behind the backend
 sub too_far_behind_backend {
     my Perlbal::ClientProxy $self    = $_[0];
-    my Perlbal::BackendHTTP $backend = $self->{backend}   or return 0;
+    my Perlbal::BackendHTTP $backend = $self->{backend};

     # if a backend doesn't have a service, it's a
     # ReproxyManager-created backend, and thus it should use the
@@ -202,7 +202,7 @@ sub too_far_behind_backend {
     # being that reproxied-to webservers are event-based and it's okay
     # to tie the up longer in favor of using less buffer memory in
     # perlbal)
-    my $max_buffer = defined $backend->{service} ?
+    my $max_buffer = ($backend && defined $backend->{service}) ?
         $self->{service}->{buffer_size} :
         $self->{service}->{buffer_size_reproxy_url};


Also, it seems that Backends inherit the max_idle_time of zero, which
means they never time out. Since the client closes them, if
client-backend breaks they are leaked. This patch allows them to be
reaped after 1/2 an hour of idle time:

diff --git a/lib/Perlbal/BackendHTTP.pm b/lib/Perlbal/BackendHTTP.pm
index 37d15cf..2eae0c4 100644
--- a/lib/Perlbal/BackendHTTP.pm
+++ b/lib/Perlbal/BackendHTTP.pm
@@ -125,6 +125,9 @@ sub init {
     $self->{buffered_upload_mode} = 0;
 }

+# Clean up inactive backends after 1/2 hour, in case of leaks
+sub max_idle_time { return 1800; }
+

 sub new_process {
     my ($class, $svc, $prog) = @_;


Lastly, if the client has lost the backend (but not vice-versa), this
patch allows the backend to detect that the client has closed and so
close itself down:

diff --git a/lib/Perlbal/BackendHTTP.pm b/lib/Perlbal/BackendHTTP.pm
index 2eae0c4..cbd7be6 100644
--- a/lib/Perlbal/BackendHTTP.pm
+++ b/lib/Perlbal/BackendHTTP.pm
@@ -545,7 +545,8 @@ sub event_read {
     # to use the end of the stream, or because a bad request error,
     # which I can't totally understand.  in any case, we have
     # no client so all we can do is close this backend.
-    return $self->close('read_with_no_client') unless $client;
+    return $self->close('read_with_no_client')
+        if (!$client || $client->{closed});

     unless ($self->{res_headers}) {
         return unless $self->read_response_headers;


I'd be grateful if someone could comment on these patches and whether
or not they're suitable for me to commit (separately, with CHANGES)
as-is?

These changes are all band-aids of various sorts, so I guess people
might object to them on those grounds (we should fix the problem
instead) but they've made a real difference in our environment.

regards,

jb