From 5eade35257aeb1302f57c112123ba7042c062b3e Mon Sep 17 00:00:00 2001
From: ridiculousfish <corydoras@ridiculousfish.com>
Date: Sun, 24 Mar 2019 21:12:41 -0700
Subject: [PATCH] Stop buffering deferred function processes

If a function process is deferred, allow it to be unbuffered.
This permits certain simple cases where functions are piped to external
commands to execute without buffering.

This is a somewhat-hacky stopgap measure that can't really be extended
to more general concurrent processes. However it is overall an improvement
in user experience that might help flush out some bugs too.
---
 src/exec.cpp    | 26 +++++++++++++++++---------
 tests/test1.in  |  2 +-
 tests/test1.out |  2 +-
 3 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/src/exec.cpp b/src/exec.cpp
index 4775dc27b..ddf612d9d 100644
--- a/src/exec.cpp
+++ b/src/exec.cpp
@@ -783,15 +783,18 @@ static bool exec_external_command(env_stack_t &vars, const std::shared_ptr<job_t
 
 /// Execute a block node or function "process".
 /// \p user_ios contains the list of user-specified ios, used so we can avoid stomping on them with
-/// our pipes. \return true on success, false on error.
+/// our pipes.
+/// \p allow_buffering if true, permit buffering the output.
+/// \return true on success, false on error.
 static bool exec_block_or_func_process(parser_t &parser, std::shared_ptr<job_t> j, process_t *p,
-                                       const io_chain_t &user_ios, io_chain_t io_chain) {
+                                       const io_chain_t &user_ios, io_chain_t io_chain,
+                                       bool allow_buffering) {
     assert((p->type == process_type_t::function || p->type == process_type_t::block_node) &&
            "Unexpected process type");
 
     // Create an output buffer if we're piping to another process.
     shared_ptr<io_bufferfill_t> block_output_bufferfill{};
-    if (!p->is_last_in_job) {
+    if (!p->is_last_in_job && allow_buffering) {
         // Be careful to handle failure, e.g. too many open fds.
         block_output_bufferfill = io_bufferfill_t::create(user_ios);
         if (!block_output_bufferfill) {
@@ -863,12 +866,13 @@ static bool exec_block_or_func_process(parser_t &parser, std::shared_ptr<job_t>
 
 /// Executes a process \p in job \j, using the pipes \p pipes (which may have invalid fds if this is
 /// the first or last process).
-/// deferred_pipes represents the pipes from our deferred process; if set ensure they get closed in
-/// any child.
-/// \returns true on success, false on exec error.
+/// \p deferred_pipes represents the pipes from our deferred process; if set ensure they get closed
+/// in any child. If \p is_deferred_run is true, then this is a deferred run; this affects how
+/// certain buffering works. \returns true on success, false on exec error.
 static bool exec_process_in_job(parser_t &parser, process_t *p, std::shared_ptr<job_t> j,
                                 autoclose_pipes_t pipes, const io_chain_t &all_ios,
-                                const autoclose_pipes_t &deferred_pipes, size_t stdout_read_limit) {
+                                const autoclose_pipes_t &deferred_pipes, size_t stdout_read_limit,
+                                bool is_deferred_run = false) {
     // The write pipe (destined for stdout) needs to occur before redirections. For example,
     // with a redirection like this:
     //
@@ -942,7 +946,11 @@ static bool exec_process_in_job(parser_t &parser, process_t *p, std::shared_ptr<
     switch (p->type) {
         case process_type_t::function:
         case process_type_t::block_node: {
-            if (!exec_block_or_func_process(parser, j, p, all_ios, process_net_io_chain)) {
+            // Allow buffering unless this is a deferred run. If deferred, then processes after us
+            // were already launched, so they are ready to receive (or reject) our output.
+            bool allow_buffering = !is_deferred_run;
+            if (!exec_block_or_func_process(parser, j, p, all_ios, process_net_io_chain,
+                                            allow_buffering)) {
                 return false;
             }
             break;
@@ -1096,7 +1104,7 @@ bool exec_job(parser_t &parser, shared_ptr<job_t> j) {
     if (!exec_error && deferred_process) {
         assert(deferred_pipes.write.valid() && "Deferred process should always have a write pipe");
         if (!exec_process_in_job(parser, deferred_process, j, std::move(deferred_pipes), all_ios,
-                                 {}, stdout_read_limit)) {
+                                 {}, stdout_read_limit, true)) {
             exec_error = true;
         }
     }
diff --git a/tests/test1.in b/tests/test1.in
index f006e774a..41fc71caa 100644
--- a/tests/test1.in
+++ b/tests/test1.in
@@ -109,7 +109,7 @@ end
 echo Test 5 $sta
 
 logmsg Verify that we can turn stderr into stdout and then pipe it
-# Note that the order here seems unspecified - 'errput' appears before 'output', why?
+# Note that the order here has historically been unspecified - 'errput' could conceivably appear before 'output'.
 begin ; echo output ; echo errput 1>&2  ; end 2>&1 | tee ../test/temp/tee_test.txt ; cat ../test/temp/tee_test.txt
 
 logmsg "Test that trailing ^ doesn't trigger redirection, see #1873"
diff --git a/tests/test1.out b/tests/test1.out
index 6c141412d..8ac15efc6 100644
--- a/tests/test1.out
+++ b/tests/test1.out
@@ -44,10 +44,10 @@ Test 5 pass
 
 ####################
 # Verify that we can turn stderr into stdout and then pipe it
-errput
 output
 errput
 output
+errput
 
 ####################
 # Test that trailing ^ doesn't trigger redirection, see #1873