Stop printing when stdout is closed.

Stop printing when stdout is closed.

This makes sure we don't print(...) data to a file opened after stdout
was closed.

BUG=https://code.google.com/p/dart/issues/detail?id=9615
R=kustermann@google.com, whesse@google.com

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

[Anders Johnsen, 2013-06-27 11:08:34]

Hang on, I'll have to fix the volatile stuff. I was thinking about the Java
version of the keyword :)

[Anders Johnsen, 2013-06-27 11:31:26]

PTAL

[kustermann, 2013-06-27 11:51:09]

In general I like the idea of using "/dev/null" as a placeholder if we close
stdout.

Maybe you could implement this a bit simpler, as in this pseudocode:
if fd == STDOUT_FD:
  devnullFd = open("/dev/null", 'w')
  // this will have a sideffect of closing STDOUT_FD and replacing it with
devnullFd
  dup(devnullFd, STDOUT_FD)
  close(devnullFd)
else:
  as usual

https://codereview.chromium.org/18031003/diff/7001/runtime/bin/eventhandler_a...
File runtime/bin/eventhandler_android.cc (right):

https://codereview.chromium.org/18031003/diff/7001/runtime/bin/eventhandler_a...
runtime/bin/eventhandler_android.cc:220: if (fd == fileno(stdout))
USE(fopen("/dev/null", "r"));
A few questions here:
 - can we safely use fileno() here? Man page says
  ERRORS ...
      (However, in case fileno() detects that its argument is  not  a
       valid stream, it must return -1 and set errno to EBADF.)
 - are we garanteed that the fopen() will use the stdout fd (Seems we're relying
on an implementation detail here. Maybe we should check and if it's not the case
use "dup()")
 - I don't know how "FILE *stdout"/fopen is implemented, but fopen() returns
probably a differen FILE instance than what 'stdout' is refering to. Maybe an
issue?
 - you're opening /dev/null as "r", shouldn't it be "w" ?

https://codereview.chromium.org/18031003/diff/7001/tests/standalone/io/stdout...
File tests/standalone/io/stdout_close_test.dart (right):

https://codereview.chromium.org/18031003/diff/7001/tests/standalone/io/stdout...
tests/standalone/io/stdout_close_test.dart:10: var dir = new
Directory('').createTempSync();
Move that line above, in case createTempSync() uses a file descriptor.

https://codereview.chromium.org/18031003/diff/7001/tests/standalone/io/stdout...
tests/standalone/io/stdout_close_test.dart:11: var file = new File('dev/file');
Did you mean 'new File("$dir/file")?

https://codereview.chromium.org/18031003/diff/7001/tests/standalone/io/stdout...
tests/standalone/io/stdout_close_test.dart:15: Expect.equals("",
file.readAsStringSync());
If this expect throws, we leave a temporary directory (+ a file) behind. Maybe
cleaning up in a try { } finally {}?

[Anders Johnsen, 2013-06-27 14:45:52]

Thanks for the feedback Martin, much better now!

PTAL

https://codereview.chromium.org/18031003/diff/7001/runtime/bin/eventhandler_a...
File runtime/bin/eventhandler_android.cc (right):

https://codereview.chromium.org/18031003/diff/7001/runtime/bin/eventhandler_a...
runtime/bin/eventhandler_android.cc:220: if (fd == fileno(stdout))
USE(fopen("/dev/null", "r"));
On 2013/06/27 11:51:09, kustermann wrote:
> A few questions here:
>  - can we safely use fileno() here? Man page says
>   ERRORS ...
>       (However, in case fileno() detects that its argument is  not  a
>        valid stream, it must return -1 and set errno to EBADF.)
>  - are we garanteed that the fopen() will use the stdout fd (Seems we're
relying
> on an implementation detail here. Maybe we should check and if it's not the
case
> use "dup()")
>  - I don't know how "FILE *stdout"/fopen is implemented, but fopen() returns
> probably a differen FILE instance than what 'stdout' is refering to. Maybe an
> issue?
>  - you're opening /dev/null as "r", shouldn't it be "w" ?

Done.

https://codereview.chromium.org/18031003/diff/7001/tests/standalone/io/stdout...
File tests/standalone/io/stdout_close_test.dart (right):

https://codereview.chromium.org/18031003/diff/7001/tests/standalone/io/stdout...
tests/standalone/io/stdout_close_test.dart:10: var dir = new
Directory('').createTempSync();
On 2013/06/27 11:51:09, kustermann wrote:
> Move that line above, in case createTempSync() uses a file descriptor.

Done.

https://codereview.chromium.org/18031003/diff/7001/tests/standalone/io/stdout...
tests/standalone/io/stdout_close_test.dart:11: var file = new File('dev/file');
On 2013/06/27 11:51:09, kustermann wrote:
> Did you mean 'new File("$dir/file")?

Done.

https://codereview.chromium.org/18031003/diff/7001/tests/standalone/io/stdout...
tests/standalone/io/stdout_close_test.dart:15: Expect.equals("",
file.readAsStringSync());
On 2013/06/27 11:51:09, kustermann wrote:
> If this expect throws, we leave a temporary directory (+ a file) behind. Maybe
> cleaning up in a try { } finally {}?

Done.

[kustermann, 2013-06-27 15:14:07]

lgtm with comments.

But let the more experienced IO guys take a look as well.

https://codereview.chromium.org/18031003/diff/13001/runtime/bin/eventhandler_...
File runtime/bin/eventhandler_android.cc (right):

https://codereview.chromium.org/18031003/diff/13001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_android.cc:220: USE(dup2(open("/dev/null", O_WRONLY),
STDOUT_FILENO));
You leak one file descriptor here. I'd do something like:

// Close the old stdout descriptor and replace it with /dev/null.
int nullFd = TEMP_FAILURE_RETRY(open("/dev/null", O_WRONLY));
ASSERT(nullFd >= 0);
VOID_TEMP_FAILURE_RETRY(dup2(nullFd, STDOUT_FILENO));
VOID_TEMP_FAILURE_RETRY(close(nullFd));

Note the last line. After dup2()'ing you've got two identical file descriptors
(one in nullFd and one in STDOUT_FILENO) so we need to close 'nullFd'.

https://codereview.chromium.org/18031003/diff/13001/runtime/bin/eventhandler_...
File runtime/bin/eventhandler_linux.cc (right):

https://codereview.chromium.org/18031003/diff/13001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_linux.cc:17: #include <fcntl.h>  // NOLINT
If you need this here, you probably also need it for _android.cc.

[Anders Johnsen, 2013-06-27 16:32:07]

Bill, can you take a look.

Martin, thanks a ton for the reviews!

https://codereview.chromium.org/18031003/diff/13001/runtime/bin/eventhandler_...
File runtime/bin/eventhandler_android.cc (right):

https://codereview.chromium.org/18031003/diff/13001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_android.cc:220: USE(dup2(open("/dev/null", O_WRONLY),
STDOUT_FILENO));
On 2013/06/27 15:14:07, kustermann wrote:
> You leak one file descriptor here. I'd do something like:
> 
> // Close the old stdout descriptor and replace it with /dev/null.
> int nullFd = TEMP_FAILURE_RETRY(open("/dev/null", O_WRONLY));
> ASSERT(nullFd >= 0);
> VOID_TEMP_FAILURE_RETRY(dup2(nullFd, STDOUT_FILENO));
> VOID_TEMP_FAILURE_RETRY(close(nullFd));
> 
> Note the last line. After dup2()'ing you've got two identical file descriptors
> (one in nullFd and one in STDOUT_FILENO) so we need to close 'nullFd'.

You're right, thanks!

https://codereview.chromium.org/18031003/diff/13001/runtime/bin/eventhandler_...
File runtime/bin/eventhandler_linux.cc (right):

https://codereview.chromium.org/18031003/diff/13001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_linux.cc:17: #include <fcntl.h>  // NOLINT
On 2013/06/27 15:14:07, kustermann wrote:
> If you need this here, you probably also need it for _android.cc.

Done.

[Bill Hesse, 2013-06-27 16:42:36]

lgtm

https://codereview.chromium.org/18031003/diff/13001/runtime/bin/eventhandler_...
File runtime/bin/eventhandler_android.cc (right):

https://codereview.chromium.org/18031003/diff/13001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_android.cc:221: } else {
Shouldn't you be checking that dev/null didn't open on STDOUT_FILENO, if you
close it?

https://codereview.chromium.org/18031003/diff/19001/runtime/bin/eventhandler_...
File runtime/bin/eventhandler_android.cc (right):

https://codereview.chromium.org/18031003/diff/19001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_android.cc:223: VOID_TEMP_FAILURE_RETRY(dup2(null_fd,
STDOUT_FILENO));
OK, so stdout is still open at this point, so we know null_fd != STDOUT_FILENO?

[Anders Johnsen, 2013-06-27 16:44:11]

Yep :)

https://codereview.chromium.org/18031003/diff/13001/runtime/bin/eventhandler_...
File runtime/bin/eventhandler_android.cc (right):

https://codereview.chromium.org/18031003/diff/13001/runtime/bin/eventhandler_...
runtime/bin/eventhandler_android.cc:221: } else {
On 2013/06/27 16:42:36, Bill Hesse wrote:
> Shouldn't you be checking that dev/null didn't open on STDOUT_FILENO, if you
> close it?

No, it's still open at this point.

[Anders Johnsen, 2013-06-27 16:44:48]

Committed patchset #6 manually as r24525 (presubmit successful).