Add support for file locking

Add support for file locking

This adds support for file locking in dart:io.

BUG=http://dartbug.com/17045
R=kustermann@google.com, lrn@google.com

Committed: https://code.google.com/p/dart/source/detail?r=42733

[Lasse Reichstein Nielsen, 2015-01-08 11:59:22]

lgtm

https://codereview.chromium.org/833623004/diff/80001/runtime/bin/file.cc
File runtime/bin/file.cc (right):

https://codereview.chromium.org/833623004/diff/80001/runtime/bin/file.cc#newc...
runtime/bin/file.cc:397: int64_t lock;
Is this the type on all platforms? There is no lock_t type to use instead?

https://codereview.chromium.org/833623004/diff/80001/runtime/bin/file.cc#newc...
runtime/bin/file.cc:398: // Lock type arguments are checked in Dart code to be
legal.
legal -> valid.
It's not a law.

Don't trust Dart code, always check again in C code.

https://codereview.chromium.org/833623004/diff/80001/runtime/bin/file_win.cc
File runtime/bin/file_win.cc (right):

https://codereview.chromium.org/833623004/diff/80001/runtime/bin/file_win.cc#...
runtime/bin/file_win.cc:139: default:
Could these two cases be combined - they only differ on the value of the second
argument, maybe just sett a variable with that argument.

https://codereview.chromium.org/833623004/diff/80001/sdk/lib/io/file.dart
File sdk/lib/io/file.dart (right):

https://codereview.chromium.org/833623004/diff/80001/sdk/lib/io/file.dart#new...
sdk/lib/io/file.dart:36: class FileLock {
Make this an enum.

https://codereview.chromium.org/833623004/diff/80001/sdk/lib/io/file.dart#new...
sdk/lib/io/file.dart:731: * *any* file descriptor for that file is closed by the
process.
Worth mentioning that advisory locks are not actually locking the file?

Also worth mentioning is that this is a per-process lock, so you share locks and
unlocks with everybody else in the same process, including other isolates.

https://codereview.chromium.org/833623004/diff/80001/sdk/lib/io/file.dart#new...
sdk/lib/io/file.dart:738: [int start, int end, FileLock mode =
FileLock.EXCLUSIVE]);
As discussed, move mode to first argument.
Maybe omit start/end, especially if it doesn't mean the same thing between
Unixes and Windows.

Maybe default start to 0, as other range functions.

https://codereview.chromium.org/833623004/diff/80001/sdk/lib/io/file.dart#new...
sdk/lib/io/file.dart:781: Future<RandomAccessFile> unlock([int start, int end]);
Maybe default start to 0.

https://codereview.chromium.org/833623004/diff/80001/tests/standalone/io/file...
File tests/standalone/io/file_lock_script.dart (right):

https://codereview.chromium.org/833623004/diff/80001/tests/standalone/io/file...
tests/standalone/io/file_lock_script.dart:1: // Copyright (c) 2013, the Dart
project authors.  Please see the AUTHORS file
2015

https://codereview.chromium.org/833623004/diff/80001/tests/standalone/io/file...
tests/standalone/io/file_lock_script.dart:13: var  mode = FileLock.EXCLUSIVE;
Consider making arguments optional, so:
  if (args.length > 1 && args[1] != 'null') ...

https://codereview.chromium.org/833623004/diff/80001/tests/standalone/io/file...
File tests/standalone/io/file_lock_test.dart (right):

https://codereview.chromium.org/833623004/diff/80001/tests/standalone/io/file...
tests/standalone/io/file_lock_test.dart:1: // Copyright (c) 2013, the Dart
project authors.  Please see the AUTHORS file
2015

https://codereview.chromium.org/833623004/diff/80001/tests/standalone/io/file...
tests/standalone/io/file_lock_test.dart:12: // Check that the file is locked or
not.
that -> whether

https://codereview.chromium.org/833623004/diff/80001/tests/standalone/io/file...
tests/standalone/io/file_lock_test.dart:270: testLockSharedAsync();
What happens if the lock range is larger than the file (end > file length)?
Document and test it.

[kustermann, 2015-01-08 12:54:28]

lgtm

https://codereview.chromium.org/833623004/diff/80001/runtime/bin/file_android.cc
File runtime/bin/file_android.cc (right):

https://codereview.chromium.org/833623004/diff/80001/runtime/bin/file_android...
runtime/bin/file_android.cc:115: ASSERT(handle_->fd() >= 0);
ASSERT(end > start);

https://codereview.chromium.org/833623004/diff/80001/runtime/bin/file_android...
runtime/bin/file_android.cc:133: int rc = fcntl(handle_->fd(), F_SETLK, &fl);
Add a comment that F_SETLK does not block but rather returns with
EACCESS/EAGAIN.

https://codereview.chromium.org/833623004/diff/80001/runtime/bin/file_win.cc
File runtime/bin/file_win.cc (right):

https://codereview.chromium.org/833623004/diff/80001/runtime/bin/file_win.cc#...
runtime/bin/file_win.cc:140: return false;
This should be UNREACHABLE()

https://codereview.chromium.org/833623004/diff/80001/runtime/bin/file_win.cc#...
runtime/bin/file_win.cc:141: }
Would it make sense to use GetLastError() if rc == FALSE?

https://codereview.chromium.org/833623004/diff/80001/sdk/lib/io/file.dart
File sdk/lib/io/file.dart (right):

https://codereview.chromium.org/833623004/diff/80001/sdk/lib/io/file.dart#new...
sdk/lib/io/file.dart:41: /// Exclusive file lock.
empty line before '///'.

https://codereview.chromium.org/833623004/diff/80001/sdk/lib/io/file.dart#new...
sdk/lib/io/file.dart:781: Future<RandomAccessFile> unlock([int start, int end]);
Why is lockSync(), unlockSync() returning void and lock()/unlock() returning not
Future but Future<RandomAccessFile>???

This is not symmetric, and what it returns must be the same instance as the
where the methods are called anyway, righ?

https://codereview.chromium.org/833623004/diff/80001/sdk/lib/io/file_impl.dart
File sdk/lib/io/file_impl.dart (right):

https://codereview.chromium.org/833623004/diff/80001/sdk/lib/io/file_impl.dar...
sdk/lib/io/file_impl.dart:948: [int start, int end, FileLock mode =
FileLock.EXCLUSIVE]) {
Move the enum value as first argument or make named arguments.

https://codereview.chromium.org/833623004/diff/80001/sdk/lib/io/file_impl.dar...
sdk/lib/io/file_impl.dart:957: int lock = mode == FileLock.EXCLUSIVE ?
LOCK_EXCLUSIVE : LOCK_SHARED;
Please assert always that end > start! - also further down.

Maybe we should change it to just be start/length ?

https://codereview.chromium.org/833623004/diff/80001/sdk/lib/io/file_impl.dar...
sdk/lib/io/file_impl.dart:963: return this;
Indentation is a bit screwed up I think (also for the next one). Move the then()
to previous line?

https://codereview.chromium.org/833623004/diff/80001/tests/standalone/io/file...
File tests/standalone/io/file_lock_script.dart (right):

https://codereview.chromium.org/833623004/diff/80001/tests/standalone/io/file...
tests/standalone/io/file_lock_script.dart:33: raf.closeSync();
move the raf.closeSync() into a finally? - then you don't need it twice.

https://codereview.chromium.org/833623004/diff/80001/tests/standalone/io/file...
File tests/standalone/io/file_lock_test.dart (right):

https://codereview.chromium.org/833623004/diff/80001/tests/standalone/io/file...
tests/standalone/io/file_lock_test.dart:57: directory.deleteSync(recursive:
true);
Could you do the directory.deleteSync() in a whenComplete(() {}) which is
guaranteed to be executed. This way we never leak temp dirs.

[same for other tests]

[Søren Gjesse, 2015-01-09 13:06:21]

https://codereview.chromium.org/833623004/diff/80001/runtime/bin/file.cc
File runtime/bin/file.cc (right):

https://codereview.chromium.org/833623004/diff/80001/runtime/bin/file.cc#newc...
runtime/bin/file.cc:397: int64_t lock;
On 2015/01/08 11:59:22, Lasse Reichstein Nielsen wrote:
> Is this the type on all platforms? There is no lock_t type to use instead?

The int64_t type is used to get the Dart int argument into C++ code. Below it is
checked to be in the range [0, 2] and then cast to the C++ enum File::LockType
which is then converted to the concrete platform type.

https://codereview.chromium.org/833623004/diff/80001/runtime/bin/file.cc#newc...
runtime/bin/file.cc:398: // Lock type arguments are checked in Dart code to be
legal.
On 2015/01/08 11:59:21, Lasse Reichstein Nielsen wrote:
> legal -> valid.
> It's not a law.
> 
> Don't trust Dart code, always check again in C code.

I an also checking here. It they are not integers the "Illegal argument" below
will be thrown.

https://codereview.chromium.org/833623004/diff/80001/runtime/bin/file_android.cc
File runtime/bin/file_android.cc (right):

https://codereview.chromium.org/833623004/diff/80001/runtime/bin/file_android...
runtime/bin/file_android.cc:115: ASSERT(handle_->fd() >= 0);
On 2015/01/08 12:54:27, kustermann wrote:
> ASSERT(end > start);

Done.

https://codereview.chromium.org/833623004/diff/80001/runtime/bin/file_android...
runtime/bin/file_android.cc:133: int rc = fcntl(handle_->fd(), F_SETLK, &fl);
On 2015/01/08 12:54:27, kustermann wrote:
> Add a comment that F_SETLK does not block but rather returns with
> EACCESS/EAGAIN.

Done.

https://codereview.chromium.org/833623004/diff/80001/runtime/bin/file_win.cc
File runtime/bin/file_win.cc (right):

https://codereview.chromium.org/833623004/diff/80001/runtime/bin/file_win.cc#...
runtime/bin/file_win.cc:139: default:
On 2015/01/08 11:59:22, Lasse Reichstein Nielsen wrote:
> Could these two cases be combined - they only differ on the value of the
second
> argument, maybe just sett a variable with that argument.

Done.

https://codereview.chromium.org/833623004/diff/80001/runtime/bin/file_win.cc#...
runtime/bin/file_win.cc:140: return false;
On 2015/01/08 12:54:27, kustermann wrote:
> This should be UNREACHABLE()

Done.

https://codereview.chromium.org/833623004/diff/80001/runtime/bin/file_win.cc#...
runtime/bin/file_win.cc:141: }
On 2015/01/08 12:54:27, kustermann wrote:
> Would it make sense to use GetLastError() if rc == FALSE?

When this results in an OS error in Dart the error code from GetLastError is
already returned.

https://codereview.chromium.org/833623004/diff/80001/sdk/lib/io/file.dart
File sdk/lib/io/file.dart (right):

https://codereview.chromium.org/833623004/diff/80001/sdk/lib/io/file.dart#new...
sdk/lib/io/file.dart:36: class FileLock {
On 2015/01/08 11:59:22, Lasse Reichstein Nielsen wrote:
> Make this an enum.

Not yet. Enums still not enabled by default in dart2js.

https://codereview.chromium.org/833623004/diff/80001/sdk/lib/io/file.dart#new...
sdk/lib/io/file.dart:41: /// Exclusive file lock.
On 2015/01/08 12:54:27, kustermann wrote:
> empty line before '///'.

Done.

https://codereview.chromium.org/833623004/diff/80001/sdk/lib/io/file.dart#new...
sdk/lib/io/file.dart:731: * *any* file descriptor for that file is closed by the
process.
On 2015/01/08 11:59:22, Lasse Reichstein Nielsen wrote:
> Worth mentioning that advisory locks are not actually locking the file?
> 
> Also worth mentioning is that this is a per-process lock, so you share locks
and
> unlocks with everybody else in the same process, including other isolates.

Done.

https://codereview.chromium.org/833623004/diff/80001/sdk/lib/io/file.dart#new...
sdk/lib/io/file.dart:738: [int start, int end, FileLock mode =
FileLock.EXCLUSIVE]);
On 2015/01/08 11:59:22, Lasse Reichstein Nielsen wrote:
> As discussed, move mode to first argument.
> Maybe omit start/end, especially if it doesn't mean the same thing between
> Unixes and Windows.
> 
> Maybe default start to 0, as other range functions.

Done.

https://codereview.chromium.org/833623004/diff/80001/sdk/lib/io/file.dart#new...
sdk/lib/io/file.dart:781: Future<RandomAccessFile> unlock([int start, int end]);
On 2015/01/08 12:54:27, kustermann wrote:
> Why is lockSync(), unlockSync() returning void and lock()/unlock() returning
not
> Future but Future<RandomAccessFile>???
> 
> This is not symmetric, and what it returns must be the same instance as the
> where the methods are called anyway, righ?

This is the convention for RandomAccessFile. All async operations which does not
have a "value" completes with this.

https://codereview.chromium.org/833623004/diff/80001/sdk/lib/io/file.dart#new...
sdk/lib/io/file.dart:781: Future<RandomAccessFile> unlock([int start, int end]);
On 2015/01/08 11:59:22, Lasse Reichstein Nielsen wrote:
> Maybe default start to 0.

Done.

https://codereview.chromium.org/833623004/diff/80001/sdk/lib/io/file_impl.dart
File sdk/lib/io/file_impl.dart (right):

https://codereview.chromium.org/833623004/diff/80001/sdk/lib/io/file_impl.dar...
sdk/lib/io/file_impl.dart:948: [int start, int end, FileLock mode =
FileLock.EXCLUSIVE]) {
On 2015/01/08 12:54:27, kustermann wrote:
> Move the enum value as first argument or make named arguments.

Done.

https://codereview.chromium.org/833623004/diff/80001/sdk/lib/io/file_impl.dar...
sdk/lib/io/file_impl.dart:957: int lock = mode == FileLock.EXCLUSIVE ?
LOCK_EXCLUSIVE : LOCK_SHARED;
On 2015/01/08 12:54:27, kustermann wrote:
> Please assert always that end > start! - also further down.
> 

Done

> Maybe we should change it to just be start/length ?

We use start/end almost exclusively in the Dart libraries.

https://codereview.chromium.org/833623004/diff/80001/sdk/lib/io/file_impl.dar...
sdk/lib/io/file_impl.dart:963: return this;
On 2015/01/08 12:54:27, kustermann wrote:
> Indentation is a bit screwed up I think (also for the next one). Move the
then()
> to previous line?

Done.

https://codereview.chromium.org/833623004/diff/80001/tests/standalone/io/file...
File tests/standalone/io/file_lock_script.dart (right):

https://codereview.chromium.org/833623004/diff/80001/tests/standalone/io/file...
tests/standalone/io/file_lock_script.dart:1: // Copyright (c) 2013, the Dart
project authors.  Please see the AUTHORS file
On 2015/01/08 11:59:22, Lasse Reichstein Nielsen wrote:
> 2015

Done.

https://codereview.chromium.org/833623004/diff/80001/tests/standalone/io/file...
tests/standalone/io/file_lock_script.dart:13: var  mode = FileLock.EXCLUSIVE;
On 2015/01/08 11:59:22, Lasse Reichstein Nielsen wrote:
> Consider making arguments optional, so:
>   if (args.length > 1 && args[1] != 'null') ...

The test code that calls this script always passes all arguments.

https://codereview.chromium.org/833623004/diff/80001/tests/standalone/io/file...
tests/standalone/io/file_lock_script.dart:33: raf.closeSync();
On 2015/01/08 12:54:27, kustermann wrote:
> move the raf.closeSync() into a finally? - then you don't need it twice.

Done.

https://codereview.chromium.org/833623004/diff/80001/tests/standalone/io/file...
File tests/standalone/io/file_lock_test.dart (right):

https://codereview.chromium.org/833623004/diff/80001/tests/standalone/io/file...
tests/standalone/io/file_lock_test.dart:1: // Copyright (c) 2013, the Dart
project authors.  Please see the AUTHORS file
On 2015/01/08 11:59:22, Lasse Reichstein Nielsen wrote:
> 2015

Done.

https://codereview.chromium.org/833623004/diff/80001/tests/standalone/io/file...
tests/standalone/io/file_lock_test.dart:12: // Check that the file is locked or
not.
On 2015/01/08 11:59:22, Lasse Reichstein Nielsen wrote:
> that -> whether

Done.

https://codereview.chromium.org/833623004/diff/80001/tests/standalone/io/file...
tests/standalone/io/file_lock_test.dart:57: directory.deleteSync(recursive:
true);
On 2015/01/08 12:54:28, kustermann wrote:
> Could you do the directory.deleteSync() in a whenComplete(() {}) which is
> guaranteed to be executed. This way we never leak temp dirs.
> 
> [same for other tests]

Done.

https://codereview.chromium.org/833623004/diff/80001/tests/standalone/io/file...
tests/standalone/io/file_lock_test.dart:270: testLockSharedAsync();
On 2015/01/08 11:59:22, Lasse Reichstein Nielsen wrote:
> What happens if the lock range is larger than the file (end > file length)?
> Document and test it.

Done.

[Søren Gjesse, 2015-01-09 13:06:50]

Committed patchset #10 (id:180001) manually as 42733 (presubmit successful).

[nweiz, 2015-01-14 01:09:36]

nweiz@google.com changed reviewers:
+ nweiz@google.com

[nweiz, 2015-01-14 01:09:37]

https://codereview.chromium.org/833623004/diff/180001/sdk/lib/io/file.dart
File sdk/lib/io/file.dart (right):

https://codereview.chromium.org/833623004/diff/180001/sdk/lib/io/file.dart#ne...
sdk/lib/io/file.dart:738: * obtain an exclusive lock on the same file.
Consider also explaining the semantics of locking across isolates on Windows,
even if they're more intuitive.

https://codereview.chromium.org/833623004/diff/180001/sdk/lib/io/file.dart#ne...
sdk/lib/io/file.dart:745: [FileLock mode = FileLock.EXCLUSIVE, int start = 0,
int end]);
It would be really nice to have these be named rather than positional
parameters. If I just want to set [start], I shouldn't have to explicitly set
[mode] as well.

[Søren Gjesse, 2015-01-14 13:06:06]

https://codereview.chromium.org/833623004/diff/180001/sdk/lib/io/file.dart
File sdk/lib/io/file.dart (right):

https://codereview.chromium.org/833623004/diff/180001/sdk/lib/io/file.dart#ne...
sdk/lib/io/file.dart:738: * obtain an exclusive lock on the same file.
On 2015/01/14 01:09:37, nweiz wrote:
> Consider also explaining the semantics of locking across isolates on Windows,
> even if they're more intuitive.

Will do.

https://codereview.chromium.org/833623004/diff/180001/sdk/lib/io/file.dart#ne...
sdk/lib/io/file.dart:745: [FileLock mode = FileLock.EXCLUSIVE, int start = 0,
int end]);
On 2015/01/14 01:09:37, nweiz wrote:
> It would be really nice to have these be named rather than positional
> parameters. If I just want to set [start], I shouldn't have to explicitly set
> [mode] as well.

This was decided based on:

1. Locking the while file is the most common case, so just specifying mode
should be easy.
2. All existing dart:io methods taking start/end use positional parameters.

[nweiz, 2015-01-16 00:06:45]

https://codereview.chromium.org/833623004/diff/180001/sdk/lib/io/file.dart
File sdk/lib/io/file.dart (right):

https://codereview.chromium.org/833623004/diff/180001/sdk/lib/io/file.dart#ne...
sdk/lib/io/file.dart:743: */
Mention what happens when the lock can't be acquired. In particular, what
specific error should users check for? Is that error consistent across systems?

https://codereview.chromium.org/833623004/diff/180001/sdk/lib/io/file.dart#ne...
sdk/lib/io/file.dart:793: */
What is the Future returned by this waiting for?