Race condition with pty_echo on macOS

[wojtekmach]

I noticed the following surprising behavior on macOS, the input is echoed back even though pty_echo is not enabled:

1> exec:start(), {ok, P, I} = exec:run("tr a-z A-Z", [stdin, stdout, pty]), exec:send(I, <<"hello\n">>), timer:sleep(100), flush().
Shell got {stdout,55327,<<"hello\r\n">>}
Shell got {stdout,55327,<<"HELLO\r\n">>}
ok

On Linux, I get the expected output line only:

1> exec:start(), {ok, P, I} = exec:run("tr a-z A-Z", [stdin, stdout, pty]), exec:send(I, <<"hello\n">>), timer:sleep(100), flush().
Shell got {stdout,55327,<<"HELLO\r\n">>}
ok

It looks like pty difference between OSes and a race condition where the child hasn't disabled echo before parent started writing. This workaround seems to fix it on my machine:

diff --git a/c_src/exec_impl.cpp b/c_src/exec_impl.cpp
index 7cf8643..3551c5b 100644
--- a/c_src/exec_impl.cpp
+++ b/c_src/exec_impl.cpp
@@ -662,6 +662,13 @@ pid_t start_child(CmdOptions& op, std::string& error)

     DEBUG(debug > 1, "Spawned child pid %d", pid);

+    #ifdef __APPLE__
+    if (op.pty()) {
+       // Give child a moment to set up PTY on macOS
+       usleep(1000);
+    }
+    #endif
+
     // Either the parent or the child could use setpgid() to change
     // the process group ID of the child. However, because the scheduling
     // of the parent and child is indeterminate after a fork(), we can’t

I think this is similar in spirit to #38 that pty support hasn't been thoroughly tested on macOS.

[saleyn]

I do not think it's a good idea to insert a sleep function in the port program's code. After all, the race condition exists there for redirecting standard streams as well, since that operation cannot be made atomic. In your case, writing to child's STDIN immediately after creation should be delayed at the top level instead, though I do agree that it's odd that the problem only manifests itself on the MacOS:

1> exec:start(), {ok, P, I} = exec:run("tr a-z A-Z", [stdin, stdout, pty]), timer:sleep(100), exec:send(I, <<"hello\n">>), timer:sleep(100), flush().

[wojtekmach]

Agreed it's a terrible idea, I mentioned only that to hopefully illustrate the problem better.

[saleyn]

Thanks for bringing it up. Maybe there's some better way to handle it on MacOS, e.g. moving the pty initialization before redirecting the streams? Could you try that?

[wojtekmach]

Yes, I tried that and it didn't solve it. The suggestion I originally got from llm was to do explicit synchronization between parent and child (using a pipe) but yeah, I'm not well versed in C and those APIs so I don't feel confident sending a PR at this time.

[cocoa-xu]

I think it's expected behaviour, the echo can be turned off:

exec:start(), 
{ok, P, I} = exec:run("tr a-z A-Z", [stdin, stdout, pty]), 
ok = exec:pty_opts(I, [{echo, 0}]), 
exec:send(I, <<"hello\n">>), 
timer:sleep(100), 
flush().

Shell got {stdout,22306,<<"HELLO\r\n">>}
ok

[wojtekmach]

@cocoa-xu thanks for looking into it. My understanding is in erlexec pty echo is supposed to be off by default.

When I run this snippet all at once I get this error:

1> exec:start(),
   {ok, P, I} = exec:run("tr a-z A-Z", [stdin, stdout, pty]),
   ok = exec:pty_opts(I, [{echo, 0}]),
   exec:send(I, <<"hello\n">>),
   timer:sleep(100),
   flush().
** exception error: no match of right hand side value {error,"failed to set pty opts"}

Adding a delay fixes it though:

  1> exec:start(),
     {ok, P, I} = exec:run("tr a-z A-Z", [stdin, stdout, pty]),
+    timer:sleep(1),
     ok = exec:pty_opts(I, [{echo, 0}]),
     exec:send(I, <<"hello\n">>),
     timer:sleep(100),
     flush().
- ** exception error: no match of right hand side value {error,"failed to set pty opts"}
+ Shell got {stdout,90179,<<"HELLO\r\n">>}
+ ok

[cocoa-xu]

I just tested this code on macOS:

1> exec:start(), {ok, P, I} = exec:run("nonexistent", [stdin, stdout, pty]), exec:send(I, <<"hello\n">>), timer:sleep(1300), flush().

and guess what do we have in the output

Shell got {stdout,86325,<<"hello\r\n">>}
ok

While on Linux:

1> exec:start(), {ok, P, I} = exec:run("nonexistent", [stdin, stdout, pty]), exec:send(I, <<"hello\n">>), timer:sleep(1300), flush().
ok

Yeah...so it's really different behaviour on macOS and Linux.

@wojtekmach perhaps we can spawn the process with echo off?

[saleyn]

I don't have time to check this, but it looks like it should be possible to set the PTY flags on the slave fd prior to forking. See this. @cocoa-xu, @wojtekmach if you have time to play with it by modifying c_src/exec_impl.cpp, please let me know