Race condition between Octave 3.2.3 and unlink()

View: New views
16 Messages — Rating Filter:   Alert me  

Race condition between Octave 3.2.3 and unlink()

by sebastien.villemot :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

Hi everyone,

If I create a file foo1.m with the following content:

----------------foo1.m------------------
system("echo 1+2 > foo2.m");
foo2;
----------------------------------------

The following command will fail most of the time (but not always):

$ touch foo2.m && rm -f foo2.m && octave foo1.m

When it fails, the error message is:

error: `foo2' undefined near line 2 column 1
error: called from:
error:   foo1.m at line 2, column 1

Introducing a long-enough time delay between the rm and Octave solves
the problem:

$ touch foo2.m && rm -f foo2.m && sleep 1 && octave foo1.m

Tested on Octave 3.2.3 with Debian unstable.

I have reported this to Debian BTS (but the report is partly incorrect
since I had first thought this was a race condition between two
instances of Octave).

Best,

--
Sébastien Villemot



_______________________________________________
Bug-octave mailing list
Bug-octave@...
https://www-old.cae.wisc.edu/mailman/listinfo/bug-octave

Updating load-path cache based on modification times probably a bad idea (was: Race condition between Octave 3.2.3 and unlink())

by sebastien.villemot :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

Hi,

I think I found the culprit for what I thought was a race condition in
Octave.

The problem has to do with the way Octave 3.2 updates its load-path: it
only updates its cache of the current directory if the modification time
of the directory changes (see load_path::dir_info::update() in
src/load-path.cc).

The problem is that modification times (as returned by stat()) are
expressed in seconds; so when two changes of the directory occur within
the same second, Octave can miss the second one.

Here is an example script which demonstrates this:

---------------------------
system("echo 1+1 > foo.m");
delete("foo.m");
try
    foo;
catch
end
system("echo 1+1 > foo.m");
foo;
---------------------------

The first two lines update the modification time of the directory (file
creation then deletion). Then the call to script "foo" in the try/catch
block will obviously fail, and by the way it will update the load-path
cache.

Then it creates the file "foo.m", and tries to launch that script.

Two cases are possible:

* the creation of "foo.m" occurred within the same second than its
deletion a few lines above. The modification time of the directory is
not updated. Then on the second call to script "foo", Octave will not
update its load-path cache, and fail with an undefined symbol error

* the creation of "foo.m" occurred in a different second: Octave updates
its load-path, and everything goes well.

So on my machine this script randomly fails, depending on when it was
launched and how fast Octave performed the file system operations.

In my opinion this invalidates the idea of updating load-path cache
based on modification times.

The only workaround for the moment is to insert sleep() calls at the
right place to enforce changes in the modification times. I am applying
such a workaround for Dynare, since otherwise this bug makes it randomly
fail.

Best,

--
Sébastien Villemot
Dynare developer
Member of Debian Octave Group



_______________________________________________
Bug-octave mailing list
Bug-octave@...
https://www-old.cae.wisc.edu/mailman/listinfo/bug-octave

Re: Updating load-path cache based on modification times probably a bad idea (was: Race condition between Octave 3.2.3 and unlink())

by Thomas Weber-8 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

Hi,

On Thu, Oct 15, 2009 at 02:09:30PM +0000, Sébastien Villemot wrote:
> Hi,
>
> I think I found the culprit for what I thought was a race condition in
> Octave.
>
> The problem has to do with the way Octave 3.2 updates its load-path: it
> only updates its cache of the current directory if the modification time
> of the directory changes (see load_path::dir_info::update() in
> src/load-path.cc).

In case it's not clear: this is a real problem and not some artificially
found issue. It makes software using Octave randomly fail at build time.

        Thomas

_______________________________________________
Bug-octave mailing list
Bug-octave@...
https://www-old.cae.wisc.edu/mailman/listinfo/bug-octave

Re: Updating load-path cache based on modification times probably a bad idea (was: Race condition between Octave 3.2.3 and unlink())

by Jaroslav Hajek-2 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

2009/10/20 Thomas Weber <thomas.weber.mail@...>:

> Hi,
>
> On Thu, Oct 15, 2009 at 02:09:30PM +0000, Sébastien Villemot wrote:
>> Hi,
>>
>> I think I found the culprit for what I thought was a race condition in
>> Octave.
>>
>> The problem has to do with the way Octave 3.2 updates its load-path: it
>> only updates its cache of the current directory if the modification time
>> of the directory changes (see load_path::dir_info::update() in
>> src/load-path.cc).
>
> In case it's not clear: this is a real problem and not some artificially
> found issue. It makes software using Octave randomly fail at build time.
>
>        Thomas
>

I don't understand - what you mean by "build time"? Most software
using Octave is not dynamically creating m-files, so is not affected.
The problem is in load_path::update, which checks the directory's
modification time to decide on whether to rescan it. The resolution is
only in seconds, though. But some check is surely wanted because you
want to avoid useless rescans. I don't have a better idea. One could
even say this is a limitation of the system, which provides no way to
tell whether the directory has changed during the last second. Maybe
rehash() could ignore the stamp for some directories? But which ones?

--
RNDr. Jaroslav Hajek
computing expert & GNU Octave developer
Aeronautical Research and Test Institute (VZLU)
Prague, Czech Republic
url: www.highegg.matfyz.cz

_______________________________________________
Bug-octave mailing list
Bug-octave@...
https://www-old.cae.wisc.edu/mailman/listinfo/bug-octave

Re: Updating load-path cache based on modification times probably a bad idea (was: Race condition between Octave 3.2.3 and unlink())

by Søren Hauberg :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

tir, 20 10 2009 kl. 22:00 +0200, skrev Jaroslav Hajek:
> The problem is in load_path::update, which checks the directory's
> modification time to decide on whether to rescan it. The resolution is
> only in seconds, though. But some check is surely wanted because you
> want to avoid useless rescans. I don't have a better idea. One could
> even say this is a limitation of the system, which provides no way to
> tell whether the directory has changed during the last second. Maybe
> rehash() could ignore the stamp for some directories? But which ones?

Perhaps it would be better to use a notification system instead of
checking for file changes? Specifically, I'm thinking that we should be
able to use, say, 'inotify' to notify us that a file has changed. When
such a notification is sent Octave could then re-read the file. This
would probably also be faster than the current approach of scanning for
file changes.

Søren

_______________________________________________
Bug-octave mailing list
Bug-octave@...
https://www-old.cae.wisc.edu/mailman/listinfo/bug-octave

Re: Updating load-path cache based on modification times probably a bad idea (was: Race condition between Octave 3.2.3 and unlink())

by John W. Eaton-3 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On 20-Oct-2009, Søren Hauberg wrote:

| tir, 20 10 2009 kl. 22:00 +0200, skrev Jaroslav Hajek:
| > The problem is in load_path::update, which checks the directory's
| > modification time to decide on whether to rescan it. The resolution is
| > only in seconds, though.

It is possible to get better resolution, but only for filesystems that
support it.  Currently, I'd guess that the most widely used filesystem
on Linux systems is probably ext3, and that only has one second
resolution for file time stamps.

| But some check is surely wanted because you
| > want to avoid useless rescans. I don't have a better idea. One could
| > even say this is a limitation of the system, which provides no way to
| > tell whether the directory has changed during the last second. Maybe
| > rehash() could ignore the stamp for some directories? But which ones?
|
| Perhaps it would be better to use a notification system instead of
| checking for file changes? Specifically, I'm thinking that we should be
| able to use, say, 'inotify' to notify us that a file has changed. When
| such a notification is sent Octave could then re-read the file. This
| would probably also be faster than the current approach of scanning for
| file changes.

I agree that being notified of changes would be better than checking
timestamps, but I don't think there is any portable way to be notified
when a file changes.

If possible, I'd rather avoid solutions that only work on some
systems.

The symbol table code already includes the following:

  octave_value
  symbol_table::fcn_info::fcn_info_rep::find (const octave_value_list& args,
                                              bool local_funcs)
  {
    octave_value retval = xfind (args, local_funcs);

    if (! retval.is_defined ())
      {
        // It is possible that the user created a file on the fly since
        // the last prompt or chdir, so try updating the load path and
        // searching again.

        load_path::update ();

        retval = xfind (args, local_funcs);
      }

    return retval;
  }

So if this is not sufficient, why not?  Is it because update always
checks the time stamp?  In that case, maybe we need to have a "force"
parameter to tell load_path::update to ignore the time stamp?  That
would make this update operation slow, but it would only happen when a
symbol is not found the first time around, so I wouldn't expect it to
be a big problem.

jwe

_______________________________________________
Bug-octave mailing list
Bug-octave@...
https://www-old.cae.wisc.edu/mailman/listinfo/bug-octave

Re: Updating load-path cache based on modification times probably a bad idea (was: Race condition between Octave 3.2.3 and unlink())

by Judd Storrs :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

If we're already keeping track of when we last read a directory/file, wouldn't it be easiest to make the comparison know about the tolerance? i.e. use something equivalent to

if modtime + tolerance > cachetime
   reparse
endif

Then when the file/directory is older than the tolerance full caching would kick in?

--judd

_______________________________________________
Bug-octave mailing list
Bug-octave@...
https://www-old.cae.wisc.edu/mailman/listinfo/bug-octave

Re: Updating load-path cache based on modification times probably a bad idea (was: Race condition between Octave 3.2.3 and unlink())

by sebastien.villemot :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

Le mardi 20 octobre 2009 à 18:07 -0400, John W. Eaton a écrit :

> The symbol table code already includes the following:
>
>   octave_value
>   symbol_table::fcn_info::fcn_info_rep::find (const octave_value_list& args,
>      bool local_funcs)
>   {
>     octave_value retval = xfind (args, local_funcs);
>
>     if (! retval.is_defined ())
>       {
> // It is possible that the user created a file on the fly since
> // the last prompt or chdir, so try updating the load path and
> // searching again.
>
> load_path::update ();
>
> retval = xfind (args, local_funcs);
>       }
>
>     return retval;
>   }
>
> So if this is not sufficient, why not?  Is it because update always
> checks the time stamp?  In that case, maybe we need to have a "force"
> parameter to tell load_path::update to ignore the time stamp?  That
> would make this update operation slow, but it would only happen when a
> symbol is not found the first time around, so I wouldn't expect it to
> be a big problem.

This solution sounds good to me. It should fix the problem I encounter
with Dynare.

Also note that this granularity problem doesn't occur with Octave 3.0.
Something must have changed between the two releases, but I don't know
what since I haven't gone through Octave 3.0 source code.

Thanks,

--
Sébastien Villemot



_______________________________________________
Bug-octave mailing list
Bug-octave@...
https://www-old.cae.wisc.edu/mailman/listinfo/bug-octave

Re: Updating load-path cache based on modification times probably a bad idea (was: Race condition between Octave 3.2.3 and unlink())

by John W. Eaton-3 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On 20-Oct-2009, Judd Storrs wrote:

| If we're already keeping track of when we last read a
| directory/file,

No, we were keeping track of the timestamp on the directory, and then
re-reading the list of files if the timestamp changed.  But that fails
for a sequence like

  system ("echo 1+1 > foo.m");
  foo
  system ("echo 2+2 > bar.m");
  bar

if the second call to system doesn't change the time stamp due to
limited resolution.

| wouldn't it be easiest to make the comparison know about the tolerance? i.e.
| use something equivalent to
|
| if modtime + tolerance > cachetime
|    reparse
| endif
|
| Then when the file/directory is older than the tolerance full caching would
| kick in?

OK, this seems better than forcing all of the directories in the load
path to be read again.  So how about the following change?  It seems
to work for me.

  http://hg.savannah.gnu.org/hgweb/octave/rev/d6b2b708b6b0

jwe
_______________________________________________
Bug-octave mailing list
Bug-octave@...
https://www-old.cae.wisc.edu/mailman/listinfo/bug-octave

Re: Updating load-path cache based on modification times probably a bad idea (was: Race condition between Octave 3.2.3 and unlink())

by Judd Storrs :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

I'm still looking to see if there's a generic way to determine the file-system time resolution. I have seen stated multiple places that FAT/FAT32 is limited to a time resolution of 2 seconds. For example http://docs.python.org/library/os.html#os.stat says:

Note

The exact meaning and resolution of the st_atime, st_mtime, and st_ctime members depends on the operating system and the file system. For example, on Windows systems using the FAT or FAT32 file systems, st_mtime has 2-second resolution, and st_atime has only 1-day resolution. See your operating system documentation for details.


I'm not sure about the "on Windows" part--whether they mean that for example linux provides 1sec emulation for FAT drives? I haven't found any support for that yet. Because removable media often uses FAT, perhaps the default should be 2 sec?

--judd


_______________________________________________
Bug-octave mailing list
Bug-octave@...
https://www-old.cae.wisc.edu/mailman/listinfo/bug-octave

Re: Updating load-path cache based on modification times probably a bad idea (was: Race condition between Octave 3.2.3 and unlink())

by sebastien.villemot :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

Le mercredi 21 octobre 2009 à 10:01 -0400, John W. Eaton a écrit :
> OK, this seems better than forcing all of the directories in the load
> path to be read again.  So how about the following change?  It seems
> to work for me.
>
>   http://hg.savannah.gnu.org/hgweb/octave/rev/d6b2b708b6b0

Applied on Octave 3.2.3, this patch solves the problem, in particular
with Dynare.

Many thanks,

--
Sébastien Villemot



_______________________________________________
Bug-octave mailing list
Bug-octave@...
https://www-old.cae.wisc.edu/mailman/listinfo/bug-octave

Re: Updating load-path cache based on modification times probably a bad idea (was: Race condition between Octave 3.2.3 and unlink())

by John W. Eaton-3 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On 22-Oct-2009, S�Aibastien Villemot wrote:

| Le mercredi 21 octobre 2009 �b` 10:01 -0400, John W. Eaton a �bicrit :
| > OK, this seems better than forcing all of the directories in the load
| > path to be read again.  So how about the following change?  It seems
| > to work for me.
| >
| >   http://hg.savannah.gnu.org/hgweb/octave/rev/d6b2b708b6b0
|
| Applied on Octave 3.2.3, this patch solves the problem, in particular
| with Dynare.

The patch modifies the interfaces of some classes, so I'm not sure
whether it is OK for a future 3.2.x release.

jwe

_______________________________________________
Bug-octave mailing list
Bug-octave@...
https://www-old.cae.wisc.edu/mailman/listinfo/bug-octave

Re: Updating load-path cache based on modification times probably a bad idea (was: Race condition between Octave 3.2.3 and unlink())

by Thomas Weber-8 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Tue, Oct 20, 2009 at 10:00:52PM +0200, Jaroslav Hajek wrote:

> 2009/10/20 Thomas Weber <thomas.weber.mail@...>:
> > Hi,
> >
> > On Thu, Oct 15, 2009 at 02:09:30PM +0000, Sébastien Villemot wrote:
> >> Hi,
> >>
> >> I think I found the culprit for what I thought was a race condition in
> >> Octave.
> >>
> >> The problem has to do with the way Octave 3.2 updates its load-path: it
> >> only updates its cache of the current directory if the modification time
> >> of the directory changes (see load_path::dir_info::update() in
> >> src/load-path.cc).
> >
> > In case it's not clear: this is a real problem and not some artificially
> > found issue. It makes software using Octave randomly fail at build time.
> >
> >        Thomas
> >
>
> I don't understand - what you mean by "build time"?

The build of dynare[1] fails when running its test suite.

[1] http://www.cepremap.cnrs.fr/dynare/

        Thomas
_______________________________________________
Bug-octave mailing list
Bug-octave@...
https://www-old.cae.wisc.edu/mailman/listinfo/bug-octave

Re: Updating load-path cache based on modification times probably a bad idea (was: Race condition between Octave 3.2.3 and unlink())

by sebastien.villemot :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

Le jeudi 22 octobre 2009 à 14:18 +0000, Sébastien Villemot a écrit :
> Le mercredi 21 octobre 2009 à 10:01 -0400, John W. Eaton a écrit :
> > OK, this seems better than forcing all of the directories in the load
> > path to be read again.  So how about the following change?  It seems
> > to work for me.
> >
> >   http://hg.savannah.gnu.org/hgweb/octave/rev/d6b2b708b6b0
>
> Applied on Octave 3.2.3, this patch solves the problem, in particular
> with Dynare.

Actually when applied to Octave 3.2.3 this patch creates another bug:

* suppose you have some M-file in /tmp/foo.m
* launch Octave from another directory (say /home/user)
* At the Octave prompt type "cd /tmp" then "foo": you get an "undefined"
error...

Does this bug also occur with the development branch of Octave ? Or is
it simply that the above patch is not suited for Octave 3.2.3 ?

Best,

--
Sébastien Villemot



_______________________________________________
Bug-octave mailing list
Bug-octave@...
https://www-old.cae.wisc.edu/mailman/listinfo/bug-octave

Re: Updating load-path cache based on modification times probably a bad idea (was: Race condition between Octave 3.2.3 and unlink())

by Thomas Weber-8 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On Thu, Oct 22, 2009 at 11:30:43AM -0400, John W. Eaton wrote:

> On 22-Oct-2009, S,Aibastien Villemot wrote:
>
> | Le mercredi 21 octobre 2009 ,b` 10:01 -0400, John W. Eaton a ,bicrit :
> | > OK, this seems better than forcing all of the directories in the load
> | > path to be read again.  So how about the following change?  It seems
> | > to work for me.
> | >
> | >   http://hg.savannah.gnu.org/hgweb/octave/rev/d6b2b708b6b0
> |
> | Applied on Octave 3.2.3, this patch solves the problem, in particular
> | with Dynare.
>
> The patch modifies the interfaces of some classes, so I'm not sure
> whether it is OK for a future 3.2.x release.

I'm trying to get a solution for this on 3.2, but I have a question:


In src/load-path.cc, the method load_path::dir_info::update()
has an
        if (is_relative)
part, in which the cache of already visited directories is checked. Why
is this check only done for relative directories?

        Thomas

_______________________________________________
Bug-octave mailing list
Bug-octave@...
https://www-old.cae.wisc.edu/mailman/listinfo/bug-octave

Re: Updating load-path cache based on modification times probably a bad idea (was: Race condition between Octave 3.2.3 and unlink())

by John W. Eaton-3 :: Rate this Message:

Reply to Author | View Threaded | Show Only this Message

On  1-Nov-2009, Thomas Weber wrote:

| On Thu, Oct 22, 2009 at 11:30:43AM -0400, John W. Eaton wrote:
| > On 22-Oct-2009, S,Aibastien Villemot wrote:
| >
| > | Le mercredi 21 octobre 2009 ,b` 10:01 -0400, John W. Eaton a ,bicrit :
| > | > OK, this seems better than forcing all of the directories in the load
| > | > path to be read again.  So how about the following change?  It seems
| > | > to work for me.
| > | >
| > | >   http://hg.savannah.gnu.org/hgweb/octave/rev/d6b2b708b6b0
| > |
| > | Applied on Octave 3.2.3, this patch solves the problem, in particular
| > | with Dynare.
| >
| > The patch modifies the interfaces of some classes, so I'm not sure
| > whether it is OK for a future 3.2.x release.
|
| I'm trying to get a solution for this on 3.2, but I have a question:
|
|
| In src/load-path.cc, the method load_path::dir_info::update()
| has an
| if (is_relative)
| part, in which the cache of already visited directories is checked. Why
| is this check only done for relative directories?

Lookups are done by whatever name is in the path.  We don't want to
have to read the directory contents for relative directories unless we
haven't seen them before, so we also keep a list of directory
information indexed by absolute names.  But we only need to convert
relative names to absolute and look them up in the abs_dir_cache.
Maybe there is a better way to do this job, but I don't see it.

I checked in the following change.  It seems to work correctly for
me.  Does the code make sense now?

  http://hg.savannah.gnu.org/hgweb/octave/rev/7922a24f78c0

jwe
_______________________________________________
Bug-octave mailing list
Bug-octave@...
https://www-old.cae.wisc.edu/mailman/listinfo/bug-octave