[Catalyst] Re: Not cleaning up temporary files / HTTP::Body

[Bill Moseley]

On Wed, Jan 13, 2010 at 6:53 AM, Bill Moseley <moseley at hank.org> wrote:

> HTTP::Body::Multipart creates temporary files for uploads.  The temp files
> are created with File::Temp( UNLINK =3D> 0 ).
>

Well, this is still broken.

Yes, since 2010 HTTP::Body has been updated to have a DESTROY where it
removes files, and Catalyst::Request calls the $body->cleanup(1) attribute
to enable this feature.

But, the problem is DESTROY (see below) looks for files to remove in
$self->{upload} hash.  But, that is an empty hash until the upload is
completed.  So, if the upload is aborted (which is when you need the clean
up to work) there's nothing in the upload hash -- so nothing to unlink.

Three years ago when I first posted about this I copied
HTTP::Body::MultiPart and set UNLINK =3D> 1 on  File::Temp->new and created
my own version that overrides.    That still works.

But, if I simply modify the current H::B::MultiPart and add UNLINK=3D>1 then
the file gets deleted and I get an error. So, the fix isn't that simple any
more.  That needs some looking at.


Clearly, what we are after is deleting the temp files.  So, might as well
let File::Temp do its job.  Just have to make sure the handle doesn't go
out of scope too early.


Oh, BTW, you can't test this with the dev server.  I had to use Apache.
 With Apache chunks are sent to Catalyst.  But, when running on the dev
server it seems like the entire upload was buffered before prepare_body
gets called -- so even with a 16MB upload I could not hit "esc" to abort
the upload while HTTP::Body was processing.

sub DESTROY {
    my $self =3D shift;

    if ( $self->{cleanup} ) {
        my @temps =3D ();
        *for my $upload ( values %{ $self->{upload} } ) {*
            push @temps, map { $_->{tempname} || () }
                ( ref $upload eq 'ARRAY' ? @{$upload} : $upload );
        }
        unlink map { $_ } grep { -e $_ } @temps;  ## this is empty when
aborted
    }

}





>
> Catalyst then deletes these temp files in $c->finalize.  The problem is
> that an exception can happen and then the temp files are not deleted.
> Happens quite often, it turns out.  I seem to always see this in the logs
> at the time of the orphaned temp file:
>
> Caught exception in engine "Apache2::RequestIO::read: (104) Connection
> reset by peer
>
> Aborting an upload isn't that unusual.
>
> I can set temp directory for these uploads and use cron to clean them out,
> but I wonder if they could not be cleaned up better automatically.
>
> For example, unlink in DESTROY when the request object goes out of scope.
> Or perhaps better, don't have HTTP::Body set UNLINK =3D> 1 by default and
> when the upload object finally goes out of scope File::Temp will remove t=
he
> temp file. HTTP::Body explicitly removes the File::Temp object from the
> upload part, but I'm not sure why it needs to do that.   Why not leave the
> File::Temp object in the upload part object then let it go out of scope at
> the end of the request.
>
> Where should this be addressed?  In Catalyst or in HTTP::Body?
>
> --
> Bill Moseley
> moseley at hank.org
>



-- =

Bill Moseley
moseley at hank.org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.scsys.co.uk/pipermail/catalyst/attachments/20130314/3bcfd=
e6a/attachment.htm