[filesystem] create_directories not thread-safe?
I have an application which sometimes fails saying that it can't create a directory, because it already exists. The directory is created by fs::create_directories("run"); and the application is multi-threaded. It seems there's a race condition: if (ph.empty() || exists(ph)) return; // First create branch, by calling ourself recursively create_directories(ph.branch_path()); // Now that parent's path exists, create the directory create_directory(ph); After one thread checks that 'ph' does not exist, another one creates the same directory, so call to 'create_directory' fails. What can be done? Basically, adding a mutex for MT build and adding a warning to docs are the variants. Opinions? - Volodya
At 09:40 AM 3/5/2004, Vladimir Prus wrote:
I have an application which sometimes fails saying that it can't create a
directory, because it already exists. The directory is created by
fs::create_directories("run");
and the application is multi-threaded. It seems there's a race condition:
if (ph.empty() || exists(ph)) return;
// First create branch, by calling ourself recursively create_directories(ph.branch_path()); // Now that parent's path exists, create the directory create_directory(ph);
After one thread checks that 'ph' does not exist, another one creates the
same directory, so call to 'create_directory' fails.
What can be done? Basically, adding a mutex for MT build and adding a warning to docs are the variants. Opinions?
There is already a general race-condition warning in the docs. See www.boost.org/libs/filesystem/doc/index.htm#Race-condition. Even if the filesystem library itself were made thread-safe, the problem could still happen because the state of the external file system may be altered by some other process or other machine. Is there a issue with solving your particular problem by either using your own mutex, or catching and discarding the exception, or moving the create_directory() call to prolog code that is single-threaded? --Beman
Beman Dawes wrote:
What can be done? Basically, adding a mutex for MT build and adding a warning to docs are the variants. Opinions?
There is already a general race-condition warning in the docs. See www.boost.org/libs/filesystem/doc/index.htm#Race-condition.
True, I've missed that.
Even if the filesystem library itself were made thread-safe, the problem could still happen because the state of the external file system may be altered by some other process or other machine.
Is there a issue with solving your particular problem by either using your own mutex, or catching and discarding the exception, or moving the create_directory() call to prolog code that is single-threaded?
No issues with those, I only wanted to make sure this problem is known. After more though, I wonder if create_directories should not swallow exceptions itself. E.g. try { create_directory(ph); } catch(const filesystem_error& e) { if (e.error() == filesystem::already_exists_error) ; // do nothing else throw; } After all, caller only cares if directory exists, if it was created by another thread it's ok. - Volodya
At 01:55 AM 3/9/2004, Vladimir Prus wrote:
After more though, I wonder if create_directories should not swallow exceptions itself. E.g.
try { create_directory(ph); } catch(const filesystem_error& e) { if (e.error() == filesystem::already_exists_error) ; // do nothing else throw; }
Note that the actual implementation would not use a try/catch, but would have the same effect as your code.
After all, caller only cares if directory exists, if it was created by another thread it's ok.
That's an interesting thought. Of course, it might not be another thread but rather another process that created the directory, and that could be a sign of real trouble. But such a change would allow simpler user code. What is now written: if ( !exists( foo ) ) create_directory( foo ); becomes: create_directory( foo ); What happens when the directory existing really does constitute and error, and create_directory() doesn't throw? Will applications fail in difficult to diagnose ways? Can applications protect themselves against silent failure? It seems to me some applications will expect directory foo to be empty, and might otherwise fail, but that this isn't a commonplace problem. Applications can detect that a directory is empty, although because of possible race conditions such a test could be misleading. But that's nothing new. Overall, it seems the change would be reasonably safe. What do others think? What happens if the path exists but is not a directory? Presumably that case must still throw. Thanks, --Beman
Beman Dawes <bdawes@acm.org> wrote:
At 01:55 AM 3/9/2004, Vladimir Prus wrote:
After all, caller only cares if directory exists, if it was created by another thread it's ok.
That's an interesting thought. Of course, it might not be another thread but rather another process that created the directory, and that could be a sign of real trouble.
Exactly. Normally people use lock-files, but I personally have a use for lock-directories. If boost::filesystem starts swallowing errors, it makes it more difficult to achieve that. I would say that people who don't care whether a directory is created should just catch the exception themselves. Alternately, you could generalize make_directory to take an extra, optional argument that specifies whether to throw if the directory already exists. Of course, it should always throw if the error is something different (e.g. if the parent directory doesn't exist). Regards, Walter Landry wlandry@ucsd.edu
Walter Landry wrote:
Beman Dawes <bdawes@acm.org> wrote:
At 01:55 AM 3/9/2004, Vladimir Prus wrote:
After all, caller only cares if directory exists, if it was created by >another thread it's ok.
That's an interesting thought. Of course, it might not be another thread but rather another process that created the directory, and that could be a sign of real trouble.
Exactly. Normally people use lock-files, but I personally have a use for lock-directories. If boost::filesystem starts swallowing errors, it makes it more difficult to achieve that.
It doesn't have to swallow errors. It can simply signal some errors via exceptions and some errors via return values.
I would say that people who don't care whether a directory is created should just catch the exception themselves. Alternately, you could generalize make_directory to take an extra, optional argument that specifies whether to throw if the directory already exists. Of course, it should always throw if the error is something different (e.g. if the parent directory doesn't exist).
This almost goes without saying. create_directory(path p) post: p exists and is a directory If the postcondition cannot be satisfied, create_directory _must_ throw. But if p already exists and is a directory, the postcondition already holds, so we are free to report the "error" via a different mechanism (COM, for example, has a conventional "alternate success code" S_FALSE for such situations).
At 12:55 PM 3/9/2004, Peter Dimov wrote:
Walter Landry wrote:
Beman Dawes <bdawes@acm.org> wrote:
At 01:55 AM 3/9/2004, Vladimir Prus wrote:
After all, caller only cares if directory exists, if it was created by >another thread it's ok.
That's an interesting thought. Of course, it might not be another thread but rather another process that created the directory, and that could be a sign of real trouble.
Exactly. Normally people use lock-files, but I personally have a use for lock-directories. If boost::filesystem starts swallowing errors, it makes it more difficult to achieve that.
It doesn't have to swallow errors. It can simply signal some errors via exceptions and some errors via return values.
I would say that people who don't care whether a directory is created should just catch the exception themselves. Alternately, you could generalize make_directory to take an extra, optional argument that specifies whether to throw if the directory already exists. Of course, it should always throw if the error is something different (e.g. if the parent directory doesn't exist).
This almost goes without saying.
create_directory(path p)
post: p exists and is a directory
If the postcondition cannot be satisfied, create_directory _must_ throw. But if p already exists and is a directory, the postcondition already holds, so we are free to report the "error" via a different mechanism (COM, for example, has a conventional "alternate success code" S_FALSE for such situations).
We have a similar case with remove(), which is reported by remove() returning a bool indicating if it actually removed anything. It would be quite consistent for create_directory() to return a bool indicating if it actually had to create the directory. --Beman
Beman Dawes <bdawes@acm.org> writes:
This almost goes without saying.
create_directory(path p)
post: p exists and is a directory
If the postcondition cannot be satisfied, create_directory _must_ throw. But if p already exists and is a directory, the postcondition already holds, so we are free to report the "error" via a different mechanism (COM, for example, has a conventional "alternate success code" S_FALSE for such situations).
We have a similar case with remove(), which is reported by remove() returning a bool indicating if it actually removed anything.
It would be quite consistent for create_directory() to return a bool indicating if it actually had to create the directory.
FWIW, I think that's the right approach. -- Dave Abrahams Boost Consulting www.boost-consulting.com
David Abrahams wrote:
We have a similar case with remove(), which is reported by remove() returning a bool indicating if it actually removed anything.
It would be quite consistent for create_directory() to return a bool indicating if it actually had to create the directory.
FWIW, I think that's the right approach.
I agree, too. Returning bool far more elegant than using second parameter or two different functions. BTW, this revives old thread about better name for 'create_directories'. The best alternative name, in my opinion, was 'demand_directory', because that emphasize that directory may exist already. But if 'create_directory' is changed as now suggested, "demand" suffix in "demand_directory" will not mean anything. So, what would be better name of create_directories: create_directory_and_parents create_path demand_path ? - Volodya
At 03:25 AM 3/10/2004, Vladimir Prus wrote:
David Abrahams wrote:
We have a similar case with remove(), which is reported by remove() returning a bool indicating if it actually removed anything.
It would be quite consistent for create_directory() to return a bool indicating if it actually had to create the directory.
FWIW, I think that's the right approach.
I agree, too. Returning bool far more elegant than using second parameter or two different functions.
I'm planning to make that change, perhaps as soon as this afternoon.
BTW, this revives old thread about better name for 'create_directories'. The best alternative name, in my opinion, was 'demand_directory', because that emphasize that directory may exist already.
But if 'create_directory' is changed as now suggested, "demand" suffix in
"demand_directory" will not mean anything.
So, what would be better name of create_directories:
create_directory_and_parents create_path demand_path
None of those seem better that "create_directories". For now, I think we should just keep "create_directories". --Beman
On Tue, 09 Mar 2004 11:36:52 -0500, Beman Dawes wrote
That's an interesting thought. Of course, it might not be another thread but rather another process that created the directory, and that could be a sign of real trouble.
But such a change would allow simpler user code. What is now written:
if ( !exists( foo ) ) create_directory( foo );
becomes:
create_directory( foo );
What happens when the directory existing really does constitute and error, and create_directory() doesn't throw? Will applications fail in difficult to diagnose ways? Can applications protect themselves against silent failure?
How about something like: void create_directory ( const path& name, bool error_if_exists = false); Then the application can decide -- defaulting to nothrow.
What happens if the path exists but is not a directory? Presumably that case must still throw.
Yes, I think so. Jeff
Jeff Garland wrote:
On Tue, 09 Mar 2004 11:36:52 -0500, Beman Dawes wrote
That's an interesting thought. Of course, it might not be another thread but rather another process that created the directory, and that could be a sign of real trouble.
But such a change would allow simpler user code. What is now written:
if ( !exists( foo ) ) create_directory( foo );
becomes:
create_directory( foo );
What happens when the directory existing really does constitute and error, and create_directory() doesn't throw? Will applications fail in difficult to diagnose ways? Can applications protect themselves against silent failure?
How about something like: void create_directory ( const path& name, bool error_if_exists = false);
Then the application can decide -- defaulting to nothrow.
I agree; the application must be able to decide if the directory existing is an error condition or not. However, I don't like the optional argument. I think I would have a hard time remembering whether it was void create_directory(const path& name, bool ok_if_exists = true); or void create_directory(const path& name, bool error_if_exists = false); create_directory(path, true); // ??? Instead I would prefer two separately named functions, like create_directory() and create_directory_exists_ok(). (Well, maybe not these two names exactly, but you get the idea. ;-) ) In the past I've used the word "soft" for this kind of thing: create_directory -- throws if directory already exists soft_create_directory -- doesn't throw if directory already exists
What happens if the path exists but is not a directory? Presumably that case must still throw.
Yes, I think so.
I agree. Just my $0.02. Bob
participants (7)
-
Beman Dawes -
David Abrahams -
Jeff Garland -
Peter Dimov -
Robert Bell -
Vladimir Prus -
Walter Landry