Skip Menu |
 

This queue is for tickets about the CHI CPAN distribution.

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

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

Bug Information
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>
Download (untitled) / with headers
text/plain 641b
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
Download (untitled) / with headers
text/plain 776b
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>
Download (untitled) / with headers
text/plain 1.5k
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>
Download (untitled) / with headers
text/plain 3.8k
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
> > > >
Fixed in 0.34.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

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