stdout not drained when exitCode completes about dart_cli_script HOT 3 CLOSED

This is intended (and documented!) behavior. Among the Script class's guarantees is:

Once done completes, stdout and stderr will emit done events immediately afterwards (modulo a few microtasks).

That "modulo" is important! If you remove await new Future(() {}), your test has a race condition between the final microtasks of the Script completing and the microtasks necessary for the test case to reach expect(fakeStdout.lines, ['hello\n']);. Notably, there is not a guarantee that stdout emits a done event before exitCode completes.

from dart_cli_script.

This is intended (and documented!) behavior.

You're right!
But is it useful? Would dart_cli_script be easier to use if exitCode and done would wait for the completion of stdout and stderr before completing? Is there any benefit of the current behaviour?

I would like to have a Script API that notifies me when everything is finished and closed. This would allow me to overcome my race condition. .done feels like it is the correct method to accomplish this.
await Future((){}); does not feel like an official way.

Delay .done by one macrotask

Adding the delay, would make done more predictable. After completion, pipes could be guaranteed to be closed.
It would also make the API more future-proof, in case it requires two macrotasks at some point in the future.

    exitCode.then((code) async {
      if (_doneCompleter.isCompleted) return;

+      await Future(() {});
      if (code == 0) {
        debug("[$name] exited with exit code 0");
        _doneCompleter.complete(null);
      } else {
        debug("[$name] exited with exit code $code");
        _doneCompleter.completeError(
            ScriptException(name, code), StackTrace.current);
      }

      _closeOutputStreams();
      _extraStderrController.close();
      _stdinCloser.close();
    }, onError: _handleError);

Adding the macrotask would also make Script.kill() more predictable, always returning false after done completed.
See interrupts pipe test (the only one failing), which requires one implicit macrotask (await lines.toList()) after script.done before kill() returns false.

    test('interrupts pipe', () async {
      var controller = StreamController<List<int>>();
      var completer = Completer<void>();
      var script = controller.stream | _watchSignalsAndStdin(completer);
      var lines = script.stdout.lines;

      controller.sink.add(utf8.encode('before'));
      await pumpEventQueue();

      expect(script.kill(), true);
      await pumpEventQueue();

      controller.sink.add(utf8.encode('after'));
      await pumpEventQueue();

      expect(script.kill(), true);
      await pumpEventQueue();

      completer.complete();
      expect(script.done, throwsScriptException(143));
      expect(await lines.toList(), [
        'from a: before',
        'b: SIGTERM',
        'b: bye!',
      ]);

+      await script.done.catchError((_) {});
      expect(script.kill(), false);
    });

With the additional delay, the test can use await script.done.catchError((_) {}); before the final assertion expect(script.kill(), false); to explicitly reach the correct state.

from dart_cli_script.

Your proposed fix introduces a new race condition: if _doneCompleter is completed during the added padding macrotask, it would end up double-completing. That's certainly fixable, but it does illustrate that messing around with task ordering without having explicit data dependency paths is very error-prone. Even if we did provide a completion-order guarantee, it would be a very bad idea to rely on it.

I'm skeptical just adding a macrotask wait would actually provide as strong a guarantee as you want it to. Asynchronous programming is complex, full of timing-dependent edge cases, and difficult to reason about, so I'm generally more comfortable providing a looser guarantee that pushes users to explicitly set up future chains rather than relying on implicit timings.

from dart_cli_script.