Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dart crashes when stdout is closed prematurely #42989

Open
jathak opened this issue Aug 7, 2020 · 5 comments
Open

Dart crashes when stdout is closed prematurely #42989

jathak opened this issue Aug 7, 2020 · 5 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. customer-dart-sass library-io

Comments

@jathak
Copy link

jathak commented Aug 7, 2020

Dart SDK version: 2.9.0
OS: Linux (Ubuntu Xenial on Travis and Debian Buster locally)

We discovered this when our dartfmt Travis tasks started failing (example job).

We initially thought it was an issue with the formatter, since our other Travis tasks were fine, but we ultimately released the issue was with this line in Travis's Dart script.

It tries to run pub deps | grep -q <query> "^[|']-- dart_style " and crashes when grep exits early when it finds the line in question.

This can be replicated with the following minimal example:

import 'dart:io';

main() {
  stdout.write('a\n');
  stdout.write('b\n');
}

Running dart example.dart | grep -q a results in:

Unhandled exception:
FileSystemException: writeFrom failed, path = '' (OS Error: Broken pipe, errno = 32)
#0      _RandomAccessFile.writeFromSync (dart:io/file_impl.dart:870:7)
#1      _StdConsumer.addStream.<anonymous closure> (dart:io/stdio.dart:309:15)
#2      _RootZone.runUnaryGuarded (dart:async/zone.dart:1384:10)
#3      _BufferingStreamSubscription._sendData (dart:async/stream_impl.dart:357:11)
#4      _BufferingStreamSubscription._add (dart:async/stream_impl.dart:285:7)
#5      _SyncStreamControllerDispatch._sendData (dart:async/stream_controller.dart:808:19)
#6      _StreamController._add (dart:async/stream_controller.dart:682:7)
#7      _StreamController.add (dart:async/stream_controller.dart:624:5)
#8      _StreamSinkImpl.add (dart:io/io_sink.dart:156:17)
#9      _IOSinkImpl.write (dart:io/io_sink.dart:289:5)
#10     _StdSink.write (dart:io/stdio.dart:338:11)
#11     main (file:///tmp/example.dart:5:9)
#12     _startIsolate.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:301:19)
#13     _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:168:12)

The pub deps version only started failing in 2.9.0 (2.8.4 works fine), but the example above also fails on 2.8.4.

@a-siva
Copy link
Contributor

a-siva commented Aug 8, 2020

In your example stdout is closed and it results in the underlying write returning an error (broken pipe) and this results in the exception being thrown. This appears to work as intended. Since there is no try/catch around the code the exception is propagated out as an unhandled exception.
@jathak could you elaborate a bit more on why you consider this a bug ?

@a-siva a-siva added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-io labels Aug 8, 2020
@nex3
Copy link
Member

nex3 commented Aug 10, 2020

Because it's common for Unix pipes to close stdin early upon completing their tasks, the typical behavior for programming languages that try to write to a closed stdio stream is for the write to silently do nothing. Requiring every call to print() or stdout.writeln() to be wrapped in a try/catch just in case the output is passed to something like grep -q is burdensome and unrealistic (as evidenced by the fact that Dart team code like pub doesn't do so).

# None of these produces an error, despite stdout closing after the first line.
$ ruby -e 'puts "foo"; puts "bar"; puts "baz"' | grep -q foo
$ python -c 'print("foo"); print("bar"); print("baz")' | grep -q foo
$ node -e 'console.log("foo"); console.log("bar"); console.log("baz")' | grep -q foo

Also, this is a breaking change that's breaking real code, including the Travis Dart configuration's built-in dartfmt support. Even if the new behavior were better (which I don't think it is), it should follow the breaking change policy rather than just showing up in a release with no announcement or discussion.

@munificent
Copy link
Member

Yes, piling on to what @nex3 said: whether or not this is a bug, it is certainly a regression. Dart code that previously ran fine on the VM is now throwing an exception. At least some of that code happens to live inside a critical tool shipped with the Dart SDK (pub), and that tool itself is invoked by the CI used by many Dart packages in a way that triggers the crash.

CC @jonasfj in case the pub folks want to workaround the regression.

@jonasfj
Copy link
Member

jonasfj commented Aug 12, 2020

I'm I suppose we could workaround it with using a zone to intercept all calls to print(), but if this is a regression in dart, I'm not sure working around it in pub is wise.

This sounds like the kind of thing that could have lots of negative side-effects whenever dart programs are used as a subprocess.

@nex3
Copy link
Member

nex3 commented Oct 12, 2020

Can you prioritize this? It's completely blocking our ability to verify formatting for any of our packages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. customer-dart-sass library-io
Projects
None yet
Development

No branches or pull requests

5 participants