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:
>
>> <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
>>
>>
>>
>>
>
> 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
>
>
>
>
