Error reporting on File in dart:io

Error reporting on File in dart:io

This implements consistent error reporting on all methods on File in dart:io. The async onError method is called with an exception argument which is the same exception as would have been thrown from the corresponding sync method.

The one method on Directory which already did this has been updated as well.

R=ager@google.com

BUG=none
TEST=tests/standalone/src/io/FileTest.dart, tests/standalone/src/io/FileInvalidArgumentsTest.dart

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

[Mads Ager (google), 2012-03-08 15:04:27]

I like the direction and I think it would be good to consistently give an
exception object in case of errors.

I wonder if it would be easy to only allocate the (immutable) OSError object in
C++ on demand and pass it out as the result. That would make all result error
checking uniform. You always have to check if "result is OSError" and you never
have to allocate a Dart OSError object if there is no error? It would be great
if we could make the rare case of an error

Regarding thread-local. I think that is too complicated until we see this on
profiles? My guess would be that the thread-local lookup would be as expensive
as having a slightly larger stack frame and passing in a pointer. Anyway, it
should be easy to go there if we need to.

https://chromiumcodereview.appspot.com/9630012/diff/1/runtime/bin/file.h
File runtime/bin/file.h (right):

https://chromiumcodereview.appspot.com/9630012/diff/1/runtime/bin/file.h#newc...
runtime/bin/file.h:28: class OSError {
We should move this to some place more general.

https://chromiumcodereview.appspot.com/9630012/diff/1/runtime/bin/file_impl.dart
File runtime/bin/file_impl.dart (right):

https://chromiumcodereview.appspot.com/9630012/diff/1/runtime/bin/file_impl.d...
runtime/bin/file_impl.dart:451: //_onError("Cannot open file: ${result[0]}
${result[1]}");
Code in comments and long line.

https://chromiumcodereview.appspot.com/9630012/diff/1/runtime/bin/io.dart
File runtime/bin/io.dart (right):

https://chromiumcodereview.appspot.com/9630012/diff/1/runtime/bin/io.dart#new...
runtime/bin/io.dart:14: /** Class for holding information on an error that was
returned from the
/**
 * Class

https://chromiumcodereview.appspot.com/9630012/diff/1/runtime/bin/io.dart#new...
runtime/bin/io.dart:17: class OSError {
We should move this to a separate file instead of having it in io.dart. We
should keep io.dart as just a file containing the imports.

https://chromiumcodereview.appspot.com/9630012/diff/1/runtime/bin/io.dart#new...
runtime/bin/io.dart:39: /** Error message supplied by the operating system. null
if no message is
Ditto.

https://chromiumcodereview.appspot.com/9630012/diff/1/runtime/bin/io.dart#new...
runtime/bin/io.dart:44: /** Error code supplied by the operating system. Will
have the value
Ditto.

[Søren Gjesse, 2012-03-08 22:50:53]

Addressed all comments.

Now a Dart OSError object is only allocated when an error occurs, and the return
value usen in the sync call can be an OSError. To allocate an OSError from C++
code I need a public static function.

If this looks reasonable I will do this for all operations on File.

http://codereview.chromium.org/9630012/diff/1/runtime/bin/file.h
File runtime/bin/file.h (right):

http://codereview.chromium.org/9630012/diff/1/runtime/bin/file.h#newcode28
runtime/bin/file.h:28: class OSError {
On 2012/03/08 15:04:27, Mads Ager wrote:
> We should move this to some place more general.

Moved to utils.h, but maybe we should have platform.h in bin as well.

http://codereview.chromium.org/9630012/diff/1/runtime/bin/file_impl.dart
File runtime/bin/file_impl.dart (right):

http://codereview.chromium.org/9630012/diff/1/runtime/bin/file_impl.dart#newc...
runtime/bin/file_impl.dart:451: //_onError("Cannot open file: ${result[0]}
${result[1]}");
On 2012/03/08 15:04:27, Mads Ager wrote:
> Code in comments and long line.

Done.

http://codereview.chromium.org/9630012/diff/1/runtime/bin/io.dart
File runtime/bin/io.dart (right):

http://codereview.chromium.org/9630012/diff/1/runtime/bin/io.dart#newcode14
runtime/bin/io.dart:14: /** Class for holding information on an error that was
returned from the
On 2012/03/08 15:04:27, Mads Ager wrote:
> /**
>  * Class

Done.

http://codereview.chromium.org/9630012/diff/1/runtime/bin/io.dart#newcode17
runtime/bin/io.dart:17: class OSError {
On 2012/03/08 15:04:27, Mads Ager wrote:
> We should move this to a separate file instead of having it in io.dart. We
> should keep io.dart as just a file containing the imports.

Moved to common.dart.

http://codereview.chromium.org/9630012/diff/1/runtime/bin/io.dart#newcode39
runtime/bin/io.dart:39: /** Error message supplied by the operating system. null
if no message is
On 2012/03/08 15:04:27, Mads Ager wrote:
> Ditto.

Done.

http://codereview.chromium.org/9630012/diff/1/runtime/bin/io.dart#newcode44
runtime/bin/io.dart:44: /** Error code supplied by the operating system. Will
have the value
On 2012/03/08 15:04:27, Mads Ager wrote:
> Ditto.

Done.

[Mads Ager (google), 2012-03-09 09:40:12]

This is looking good.

The main issue is the exposed class with one static method. We should find a way
to call the constructor instead.

http://codereview.chromium.org/9630012/diff/10001/runtime/bin/common.dart
File runtime/bin/common.dart (right):

http://codereview.chromium.org/9630012/diff/10001/runtime/bin/common.dart#new...
runtime/bin/common.dart:46: static OSError makeOSError(message, code) => new
OSError(message, code);
We should get rid of this public class and public static method. We just need to
figure out how to construct an object through the API (or expose the
functionality in the API if it is not there).

http://codereview.chromium.org/9630012/diff/10001/runtime/bin/directory_impl....
File runtime/bin/directory_impl.dart (right):

http://codereview.chromium.org/9630012/diff/10001/runtime/bin/directory_impl....
runtime/bin/directory_impl.dart:86: var result = _createTemp(path, osError);
We should update this in the same way.

http://codereview.chromium.org/9630012/diff/10001/runtime/bin/file.cc
File runtime/bin/file.cc (right):

http://codereview.chromium.org/9630012/diff/10001/runtime/bin/file.cc#newcode63
runtime/bin/file.cc:63: Dart_Handle MakeDartOSError(OSError* os_error) {
This is fine for now, but we should figure out how to call a constructor through
the API (is it just a static function that we can call or do we need a new
method exposed in the API?)

http://codereview.chromium.org/9630012/diff/10001/runtime/bin/file.h
File runtime/bin/file.h (right):

http://codereview.chromium.org/9630012/diff/10001/runtime/bin/file.h#newcode117
runtime/bin/file.h:117: OSError* os_error = NULL);
I think we have avoided default arguments in the C code so far. Let's keep it
that way and make this an explicit argument?

http://codereview.chromium.org/9630012/diff/10001/runtime/bin/file_impl.dart
File runtime/bin/file_impl.dart (right):

http://codereview.chromium.org/9630012/diff/10001/runtime/bin/file_impl.dart#...
runtime/bin/file_impl.dart:254: static open(String name, int mode) native
"File_Open";
Do we want 'void' here?

http://codereview.chromium.org/9630012/diff/10001/runtime/bin/utils.h
File runtime/bin/utils.h (right):

http://codereview.chromium.org/9630012/diff/10001/runtime/bin/utils.h#newcode16
runtime/bin/utils.h:16: void SetMessage(char* message) { message_ =
strdup(message); }
Let's add an ASSERT(message_ == NULL) here to guard against memory leaks caused
by repeated SetMessage calls?

http://codereview.chromium.org/9630012/diff/10001/tests/standalone/src/io/Fil...
File tests/standalone/src/io/FileTest.dart (right):

http://codereview.chromium.org/9630012/diff/10001/tests/standalone/src/io/Fil...
tests/standalone/src/io/FileTest.dart:519: if (new Platform().operatingSystem()
== "macos") {
Remember to broaden this one to the other platforms as well when implemented
there.

[Søren Gjesse, 2012-03-13 08:25:55]

I have now refactored the change to make the error handling more consistent.

Addressed all previous comments.

Implemented on all platforms.

Renamed issue and updated description.

https://chromiumcodereview.appspot.com/9630012/diff/10001/runtime/bin/common....
File runtime/bin/common.dart (right):

https://chromiumcodereview.appspot.com/9630012/diff/10001/runtime/bin/common....
runtime/bin/common.dart:46: static OSError makeOSError(message, code) => new
OSError(message, code);
On 2012/03/09 09:40:13, Mads Ager wrote:
> We should get rid of this public class and public static method. We just need
to
> figure out how to construct an object through the API (or expose the
> functionality in the API if it is not there).

Added TODO and filed issue 2081.

https://chromiumcodereview.appspot.com/9630012/diff/10001/runtime/bin/directo...
File runtime/bin/directory_impl.dart (right):

https://chromiumcodereview.appspot.com/9630012/diff/10001/runtime/bin/directo...
runtime/bin/directory_impl.dart:86: var result = _createTemp(path, osError);
On 2012/03/09 09:40:13, Mads Ager wrote:
> We should update this in the same way.

Done.

https://chromiumcodereview.appspot.com/9630012/diff/10001/runtime/bin/file.cc
File runtime/bin/file.cc (right):

https://chromiumcodereview.appspot.com/9630012/diff/10001/runtime/bin/file.cc...
runtime/bin/file.cc:63: Dart_Handle MakeDartOSError(OSError* os_error) {
On 2012/03/09 09:40:13, Mads Ager wrote:
> This is fine for now, but we should figure out how to call a constructor
through
> the API (is it just a static function that we can call or do we need a new
> method exposed in the API?)

Opened issue http://code.google.com/p/dart/issues/detail?id=2081.

https://chromiumcodereview.appspot.com/9630012/diff/10001/runtime/bin/file.h
File runtime/bin/file.h (right):

https://chromiumcodereview.appspot.com/9630012/diff/10001/runtime/bin/file.h#...
runtime/bin/file.h:117: OSError* os_error = NULL);
On 2012/03/09 09:40:13, Mads Ager wrote:
> I think we have avoided default arguments in the C code so far. Let's keep it
> that way and make this an explicit argument?

I removed the OSError argument. The OS error is now captured from the system
after returning NULL from File::Open.

https://chromiumcodereview.appspot.com/9630012/diff/10001/runtime/bin/file_im...
File runtime/bin/file_impl.dart (right):

https://chromiumcodereview.appspot.com/9630012/diff/10001/runtime/bin/file_im...
runtime/bin/file_impl.dart:254: static open(String name, int mode) native
"File_Open";
On 2012/03/09 09:40:13, Mads Ager wrote:
> Do we want 'void' here?

No, open now returns either an int or an instance of OSError. We could write
Object.

https://chromiumcodereview.appspot.com/9630012/diff/10001/runtime/bin/utils.h
File runtime/bin/utils.h (right):

https://chromiumcodereview.appspot.com/9630012/diff/10001/runtime/bin/utils.h...
runtime/bin/utils.h:16: void SetMessage(char* message) { message_ =
strdup(message); }
On 2012/03/09 09:40:13, Mads Ager wrote:
> Let's add an ASSERT(message_ == NULL) here to guard against memory leaks
caused
> by repeated SetMessage calls?

Added freeing of message_ instead.

https://chromiumcodereview.appspot.com/9630012/diff/10001/tests/standalone/sr...
File tests/standalone/src/io/FileTest.dart (right):

https://chromiumcodereview.appspot.com/9630012/diff/10001/tests/standalone/sr...
tests/standalone/src/io/FileTest.dart:519: if (new Platform().operatingSystem()
== "macos") {
On 2012/03/09 09:40:13, Mads Ager wrote:
> Remember to broaden this one to the other platforms as well when implemented
> there.

Done.

[Mads Ager (google), 2012-03-13 10:56:17]

https://chromiumcodereview.appspot.com/9630012/diff/15001/runtime/bin/common....
File runtime/bin/common.dart (right):

https://chromiumcodereview.appspot.com/9630012/diff/15001/runtime/bin/common....
runtime/bin/common.dart:6: * Class for holding information on an error that was
returned from the
An [OSError] object holds information about an error from the operating system.

?

https://chromiumcodereview.appspot.com/9630012/diff/15001/runtime/bin/common....
runtime/bin/common.dart:10: static int noErrorCode = -1;
This should be final!

https://chromiumcodereview.appspot.com/9630012/diff/15001/runtime/bin/common....
runtime/bin/common.dart:12: const OSError([String this.message, int
this.errorCode = -1]);
The -1 here should be noErrorCode?

https://chromiumcodereview.appspot.com/9630012/diff/15001/runtime/bin/common....
runtime/bin/common.dart:17: if (message != null && message.length > 0) {
How about making this.message the empty string by default and simplify this
check to !message.isEmpty()

https://chromiumcodereview.appspot.com/9630012/diff/15001/runtime/bin/common....
runtime/bin/common.dart:45: // TODO(sgjesse): Remove this once Dart constructors
can be invoked
Please make this an actual doc comment where it says that this should not be
used. It will be removed in an upcoming change. That way it will be clear on
api.dartlang.org if this makes it to there before we can get rid of it.

https://chromiumcodereview.appspot.com/9630012/diff/15001/runtime/bin/directo...
File runtime/bin/directory.dart (right):

https://chromiumcodereview.appspot.com/9630012/diff/15001/runtime/bin/directo...
runtime/bin/directory.dart:153: if (message != null) {
!message.isEmpty()?

https://chromiumcodereview.appspot.com/9630012/diff/15001/runtime/bin/file.dart
File runtime/bin/file.dart (right):

https://chromiumcodereview.appspot.com/9630012/diff/15001/runtime/bin/file.da...
runtime/bin/file.dart:374: if (message != null) {
!message.isEmpty()?

https://chromiumcodereview.appspot.com/9630012/diff/15001/runtime/bin/file.da...
runtime/bin/file.dart:377: sb.add(" (${osError.toString()})");
You shouldn't have to explictly call toString here. String interpolation does
that for you.

https://chromiumcodereview.appspot.com/9630012/diff/15001/runtime/bin/file.da...
runtime/bin/file.dart:380: sb.add(": ${osError.toString()}");
Ditto.

https://chromiumcodereview.appspot.com/9630012/diff/15001/runtime/bin/file.h
File runtime/bin/file.h (right):

https://chromiumcodereview.appspot.com/9630012/diff/15001/runtime/bin/file.h#...
runtime/bin/file.h:22: #include "bin/utils.h"
Can we remove this from here? Nothing seems to use it?

https://chromiumcodereview.appspot.com/9630012/diff/15001/runtime/bin/file_im...
File runtime/bin/file_impl.dart (right):

https://chromiumcodereview.appspot.com/9630012/diff/15001/runtime/bin/file_im...
runtime/bin/file_impl.dart:656: bool _responseError(result, String message) {
Should we split this into two methods? _isErrorResponse and
_reportErrorResponse?

Or maybe rename to something like _reportIfErrorResponse?

https://chromiumcodereview.appspot.com/9630012/diff/15001/runtime/bin/file_li...
File runtime/bin/file_linux.cc (right):

https://chromiumcodereview.appspot.com/9630012/diff/15001/runtime/bin/file_li...
runtime/bin/file_linux.cc:109: if (TEMP_FAILURE_RETRY(stat(name, &st)) == 0) {
Why do you need this? Isn't the error code from open enough?

https://chromiumcodereview.appspot.com/9630012/diff/15001/runtime/bin/file_li...
runtime/bin/file_linux.cc:112: return false;
false -> NULL

https://chromiumcodereview.appspot.com/9630012/diff/15001/runtime/bin/file_li...
runtime/bin/file_linux.cc:189: if (TEMP_FAILURE_RETRY(stat(pathname, &st)) == 0)
{
Similarly here, don't we get a good enough error message if we do not do this?

https://chromiumcodereview.appspot.com/9630012/diff/15001/runtime/bin/utils.cc
File runtime/bin/utils.cc (right):

https://chromiumcodereview.appspot.com/9630012/diff/15001/runtime/bin/utils.c...
runtime/bin/utils.cc:1: // Copyright (c) 2012, the Dart project authors.  Please
see the AUTHORS file
Get rid of this file?

https://chromiumcodereview.appspot.com/9630012/diff/15001/runtime/bin/utils.h
File runtime/bin/utils.h (right):

https://chromiumcodereview.appspot.com/9630012/diff/15001/runtime/bin/utils.h...
runtime/bin/utils.h:28: message_ = NULL;
Redundant because of the next line.

https://chromiumcodereview.appspot.com/9630012/diff/15001/tests/standalone/sr...
File tests/standalone/src/io/DirectoryTest.dart (right):

https://chromiumcodereview.appspot.com/9630012/diff/15001/tests/standalone/sr...
tests/standalone/src/io/DirectoryTest.dart:441: Expect.throws(() => new
Directory(location).createTempSync(),
Is this change needed?

https://chromiumcodereview.appspot.com/9630012/diff/15001/tests/standalone/sr...
File tests/standalone/src/io/FileTest.dart (right):

https://chromiumcodereview.appspot.com/9630012/diff/15001/tests/standalone/sr...
tests/standalone/src/io/FileTest.dart:651: {
How about lifting the helpers to the top level and having the individual tests
as individual methods as well?

[Søren Gjesse, 2012-03-13 12:39:48]

http://codereview.chromium.org/9630012/diff/15001/runtime/bin/common.dart
File runtime/bin/common.dart (right):

http://codereview.chromium.org/9630012/diff/15001/runtime/bin/common.dart#new...
runtime/bin/common.dart:6: * Class for holding information on an error that was
returned from the
On 2012/03/13 10:56:17, Mads Ager wrote:
> An [OSError] object holds information about an error from the operating
system.
> 
> ?

Done.

http://codereview.chromium.org/9630012/diff/15001/runtime/bin/common.dart#new...
runtime/bin/common.dart:10: static int noErrorCode = -1;
On 2012/03/13 10:56:17, Mads Ager wrote:
> This should be final!

Absolutely, done.

http://codereview.chromium.org/9630012/diff/15001/runtime/bin/common.dart#new...
runtime/bin/common.dart:12: const OSError([String this.message, int
this.errorCode = -1]);
On 2012/03/13 10:56:17, Mads Ager wrote:
> The -1 here should be noErrorCode?

Yes, now that noErrorCode is final it can.

http://codereview.chromium.org/9630012/diff/15001/runtime/bin/common.dart#new...
runtime/bin/common.dart:17: if (message != null && message.length > 0) {
On 2012/03/13 10:56:17, Mads Ager wrote:
> How about making this.message the empty string by default and simplify this
> check to !message.isEmpty()

Done.

http://codereview.chromium.org/9630012/diff/15001/runtime/bin/common.dart#new...
runtime/bin/common.dart:45: // TODO(sgjesse): Remove this once Dart constructors
can be invoked
On 2012/03/13 10:56:17, Mads Ager wrote:
> Please make this an actual doc comment where it says that this should not be
> used. It will be removed in an upcoming change. That way it will be clear on
> http://api.dartlang.org if this makes it to there before we can get rid of it.

Done.

http://codereview.chromium.org/9630012/diff/15001/runtime/bin/directory.dart
File runtime/bin/directory.dart (right):

http://codereview.chromium.org/9630012/diff/15001/runtime/bin/directory.dart#...
runtime/bin/directory.dart:153: if (message != null) {
On 2012/03/13 10:56:17, Mads Ager wrote:
> !message.isEmpty()?

Done.

http://codereview.chromium.org/9630012/diff/15001/runtime/bin/file.dart
File runtime/bin/file.dart (right):

http://codereview.chromium.org/9630012/diff/15001/runtime/bin/file.dart#newco...
runtime/bin/file.dart:374: if (message != null) {
On 2012/03/13 10:56:17, Mads Ager wrote:
> !message.isEmpty()?

Done.

http://codereview.chromium.org/9630012/diff/15001/runtime/bin/file.dart#newco...
runtime/bin/file.dart:377: sb.add(" (${osError.toString()})");
On 2012/03/13 10:56:17, Mads Ager wrote:
> You shouldn't have to explictly call toString here. String interpolation does
> that for you.

Done.

http://codereview.chromium.org/9630012/diff/15001/runtime/bin/file.dart#newco...
runtime/bin/file.dart:380: sb.add(": ${osError.toString()}");
On 2012/03/13 10:56:17, Mads Ager wrote:
> Ditto.

Done.

http://codereview.chromium.org/9630012/diff/15001/runtime/bin/file.h
File runtime/bin/file.h (right):

http://codereview.chromium.org/9630012/diff/15001/runtime/bin/file.h#newcode22
runtime/bin/file.h:22: #include "bin/utils.h"
On 2012/03/13 10:56:17, Mads Ager wrote:
> Can we remove this from here? Nothing seems to use it?

Done.

http://codereview.chromium.org/9630012/diff/15001/runtime/bin/file_impl.dart
File runtime/bin/file_impl.dart (right):

http://codereview.chromium.org/9630012/diff/15001/runtime/bin/file_impl.dart#...
runtime/bin/file_impl.dart:656: bool _responseError(result, String message) {
On 2012/03/13 10:56:17, Mads Ager wrote:
> Should we split this into two methods? _isErrorResponse and
> _reportErrorResponse?
> 
> Or maybe rename to something like _reportIfErrorResponse?

Changed to two functions.

http://codereview.chromium.org/9630012/diff/15001/runtime/bin/file_linux.cc
File runtime/bin/file_linux.cc (right):

http://codereview.chromium.org/9630012/diff/15001/runtime/bin/file_linux.cc#n...
runtime/bin/file_linux.cc:109: if (TEMP_FAILURE_RETRY(stat(name, &st)) == 0) {
On 2012/03/13 10:56:17, Mads Ager wrote:
> Why do you need this? Isn't the error code from open enough?

This is to avoid opening non-regular files. Before we used exists in Dart before
calling opne to avoid opening directories, and other "special" files, but using
can eat the actual error we want to report.

We could also just allow opening directories and special files.

Added comment.

http://codereview.chromium.org/9630012/diff/15001/runtime/bin/file_linux.cc#n...
runtime/bin/file_linux.cc:112: return false;
On 2012/03/13 10:56:17, Mads Ager wrote:
> false -> NULL

Good catch, done.

http://codereview.chromium.org/9630012/diff/15001/runtime/bin/file_linux.cc#n...
runtime/bin/file_linux.cc:189: if (TEMP_FAILURE_RETRY(stat(pathname, &st)) == 0)
{
On 2012/03/13 10:56:17, Mads Ager wrote:
> Similarly here, don't we get a good enough error message if we do not do this?

As for open above we used to have an exists check in Dart. The system call
dirname is just a string operation.

Added comment.

http://codereview.chromium.org/9630012/diff/15001/runtime/bin/utils.cc
File runtime/bin/utils.cc (right):

http://codereview.chromium.org/9630012/diff/15001/runtime/bin/utils.cc#newcode1
runtime/bin/utils.cc:1: // Copyright (c) 2012, the Dart project authors.  Please
see the AUTHORS file
On 2012/03/13 10:56:17, Mads Ager wrote:
> Get rid of this file?

Absolutely - its gone.

http://codereview.chromium.org/9630012/diff/15001/runtime/bin/utils.h
File runtime/bin/utils.h (right):

http://codereview.chromium.org/9630012/diff/15001/runtime/bin/utils.h#newcode28
runtime/bin/utils.h:28: message_ = NULL;
On 2012/03/13 10:56:17, Mads Ager wrote:
> Redundant because of the next line.

Added NULL check.

http://codereview.chromium.org/9630012/diff/15001/tests/standalone/src/io/Dir...
File tests/standalone/src/io/DirectoryTest.dart (right):

http://codereview.chromium.org/9630012/diff/15001/tests/standalone/src/io/Dir...
tests/standalone/src/io/DirectoryTest.dart:441: Expect.throws(() => new
Directory(location).createTempSync(),
On 2012/03/13 10:56:17, Mads Ager wrote:
> Is this change needed?

No, reverted.

http://codereview.chromium.org/9630012/diff/15001/tests/standalone/src/io/Fil...
File tests/standalone/src/io/FileTest.dart (right):

http://codereview.chromium.org/9630012/diff/15001/tests/standalone/src/io/Fil...
tests/standalone/src/io/FileTest.dart:651: {
On 2012/03/13 10:56:17, Mads Ager wrote:
> How about lifting the helpers to the top level and having the individual tests
> as individual methods as well?

Split into individual tests and moved to a separate file FileErrorTest.dart

[Mads Ager (google), 2012-03-13 12:46:28]

lgtm