This queue is for tickets about the CHI CPAN distribution.

Report information
The Basics
Id:
50104
Status:
resolved
Priority:
Low/Low
Queue:

People
Owner:
JSWARTZ [...] cpan.org
Requestors:
bluefeet [...] gmail.com
Cc:
AdminCc:

BugTracker
Severity:
(no value)
Broken in:
(no value)
Fixed in:
(no value)



Subject: race condition in CHI::Driver::File::remove()
Date: Tue, 29 Sep 2009 10:15:09 -0700
To: bug-CHI@rt.cpan.org
From: Aran Deltac <bluefeet@gmail.com>
This code in CHI::Driver::File: sub remove { my ( $self, $key ) = @_; my $file = $self->path_to_key($key) or return undef; unlink($file); die "could not unlink '$file'" if -f $file; } There is a race condition here, which we just ran in to in production, where between the unlink and the check to see if the file exists, another process added the file back in. Things like this should be done in an atomic fashion. Consider using autodie or checking the return value of unlink, as in: if (unlink($file) != 1) { die "could not unlink '$file'"; } There may be a similar issue in clear(), but its hard to tell. Aran
Yup, good point. Will fix. Thanks. On Tue Sep 29 13:15:38 2009, bluefeet@gmail.com wrote:
Show quoted text
> This code in CHI::Driver::File: > > sub remove { > my ( $self, $key ) = @_; > > my $file = $self->path_to_key($key) or return undef; > unlink($file); > die "could not unlink '$file'" if -f $file; > } > > There is a race condition here, which we just ran in to in production, > where between the unlink and the check to see if the file exists, > another process added the file back in. Things like this should be > done in an atomic fashion. Consider using autodie or checking the > return value of unlink, as in: > > if (unlink($file) != 1) { die "could not unlink '$file'"; } > > There may be a similar issue in clear(), but its hard to tell. > > Aran
Subject: Re: [rt.cpan.org #50104] race condition in CHI::Driver::File::remove()
Date: Wed, 7 Oct 2009 15:22:57 -0700
To: bug-CHI@rt.cpan.org
From: Aran Deltac <bluefeet@gmail.com>
Hi Jonathan - turns out my fix actually wasn't correct.  We went to production with it and had an even worse time with race conditions as two processes could both believe that the cache file exists, and both attempt to unlink them at the same time.  The first one in will be all good, the second one will error.  We ended up removing all verification of whether unlink worked or not, like this:

sub remove {
    my ( $self, $key ) = @_;
    my $file = $self->path_to_key($key) or return undef;
    unlink($file);
}

I'm having a hard time coming up with a better solution.  It works for us, and I'm betting it should be the final solution to this issue.

Thanks,

Aran

On Wed, Oct 7, 2009 at 2:52 PM, Jonathan Swartz via RT <bug-CHI@rt.cpan.org> wrote:
Show quoted text
<URL: https://rt.cpan.org/Ticket/Display.html?id=50104 >

Yup, good point. Will fix. Thanks.

On Tue Sep 29 13:15:38 2009, bluefeet@gmail.com wrote:
> This code in CHI::Driver::File:
>
> sub remove {
>     my ( $self, $key ) = @_;
>
>     my $file = $self->path_to_key($key) or return undef;
>     unlink($file);
>     die "could not unlink '$file'" if -f $file;
> }
>
> There is a race condition here, which we just ran in to in production,
> where between the unlink and the check to see if the file exists,
> another process added the file back in.  Things like this should be
> done in an atomic fashion.  Consider using autodie or checking the
> return value of unlink, as in:
>
>     if (unlink($file) != 1) { die "could not unlink '$file'"; }
>
> There may be a similar issue in clear(), but its hard to tell.
>
> Aran




Subject: Re: [rt.cpan.org #50104] race condition in CHI::Driver::File::remove()
Date: Wed, 7 Oct 2009 16:04:00 -0700
To: bug-CHI@rt.cpan.org
From: Jonathan Swartz <swartz@pobox.com>
Hm. It is a shame not to verify whether the remove worked. Since remove is used to expire caches, you'd definitely want to know if that failed and left invalid cache entries around.

What about getting a cheap signature of the file (e.g. from stat()) and after removing, making sure that either (1) the file doesn't exist, or (2) it exists with a different signature? Or is this just open to other race conditions?

On Oct 7, 2009, at 3:23 PM, Aran Deltac via RT wrote:

Show quoted text
      Queue: CHI
Ticket <URL: http://rt.cpan.org/Ticket/Display.html?id=50104 >

Hi Jonathan - turns out my fix actually wasn't correct.  We went to
production with it and had an even worse time with race conditions as two
processes could both believe that the cache file exists, and both attempt to
unlink them at the same time.  The first one in will be all good, the second
one will error.  We ended up removing all verification of whether unlink
worked or not, like this:
sub remove {
   my ( $self, $key ) = @_;
   my $file = $self->path_to_key($key) or return undef;
   unlink($file);
}

I'm having a hard time coming up with a better solution.  It works for us,
and I'm betting it should be the final solution to this issue.

Thanks,

Aran

On Wed, Oct 7, 2009 at 2:52 PM, Jonathan Swartz via RT
<bug-CHI@rt.cpan.org>wrote:


Yup, good point. Will fix. Thanks.

On Tue Sep 29 13:15:38 2009, bluefeet@gmail.com wrote:
This code in CHI::Driver::File:

sub remove {
   my ( $self, $key ) = @_;

   my $file = $self->path_to_key($key) or return undef;
   unlink($file);
   die "could not unlink '$file'" if -f $file;
}

There is a race condition here, which we just ran in to in production,
where between the unlink and the check to see if the file exists,
another process added the file back in.  Things like this should be
done in an atomic fashion.  Consider using autodie or checking the
return value of unlink, as in:

   if (unlink($file) != 1) { die "could not unlink '$file'"; }

There may be a similar issue in clear(), but its hard to tell.

Aran





Hi Jonathan - turns out my fix actually wasn't correct.  We went to production with it and had an even worse time with race conditions as two processes could both believe that the cache file exists, and both attempt to unlink them at the same time.  The first one in will be all good, the second one will error.  We ended up removing all verification of whether unlink worked or not, like this:

sub remove {
    my ( $self, $key ) = @_;
    my $file = $self->path_to_key($key) or return undef;
    unlink($file);
}

I'm having a hard time coming up with a better solution.  It works for us, and I'm betting it should be the final solution to this issue.

Thanks,

Aran

On Wed, Oct 7, 2009 at 2:52 PM, Jonathan Swartz via RT <bug-CHI@rt.cpan.org> wrote:
<URL: https://rt.cpan.org/Ticket/Display.html?id=50104 >

Yup, good point. Will fix. Thanks.

On Tue Sep 29 13:15:38 2009, bluefeet@gmail.com wrote:
> This code in CHI::Driver::File:
>
> sub remove {
>     my ( $self, $key ) = @_;
>
>     my $file = $self->path_to_key($key) or return undef;
>     unlink($file);
>     die "could not unlink '$file'" if -f $file;
> }
>
> There is a race condition here, which we just ran in to in production,
> where between the unlink and the check to see if the file exists,
> another process added the file back in.  Things like this should be
> done in an atomic fashion.  Consider using autodie or checking the
> return value of unlink, as in:
>
>     if (unlink($file) != 1) { die "could not unlink '$file'"; }
>
> There may be a similar issue in clear(), but its hard to tell.
>
> Aran





Fixed in 0.34.


This service runs on Request Tracker, is sponsored by The Perl Foundation, and maintained by Best Practical Solutions.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.