[process] Output from terminal process is swallowed

[JanKoehnlein]

When a TerminalProcess is started, we spawn the command in a node-pty terminal and then connect the output. If the process is fast enough, it can produce output before it's connected, thus this output is swallowed. This is particularly bad when the process exits early with an error message.

To reproduce this, create a task that runs a shell script that just echos a string, e.g.

{
    "tasks": [
        {
            "label": "Test task",
            "type": "shell",
            "cwd": "${workspaceFolder}",
            "command": "./test.sh",
            "args": [
            ]
        }
    ]
}

#!/bin/bash
#sleep 2
echo You won\'t see me

With the sleep command commented out, you will not see any output in Theia's terminal.

[JanKoehnlein]

The solution is to pass the shell as command to the spawn method, then connect the output and finally issue the actual command with write in the constructor of TerminalProcess (as in the example from node-pty)

We could also fix that locally in the tasks API, but I assume it's better to approach this in TerminalProcess.

[svenefftinge]

Offline discussion results: It should be fixed in the 'tasks' implementation. It should use ShellProcess instead of TerminalProcess and not pass a shell. But instead send the command through write on the returned object. cc @marcdumais-work

[simark]

The problem we identified in #2961 was that for a short lived process, we registered it (on creation) and unregister it (on exit) very quickly from the process manager. By the time the terminal finally opens in the frontend, the process has been unregistered, and the terminal just runs an interactive shell as a fallback (I guess that's what you see?).

The solution we had in mind was more to either

1. somehow wait for the terminal to be attached before running the process
2. keep the output buffer around until we consume it

I would tend towards option 2.

[JanKoehnlein]

Sounds like a different problem: What I am trying to fix is that the output buffer does not get populated at all because the command has already finished before the output buffer is populated. The solution to this is

1. start the shell
2. attach the output buffer
3. run command

instead of

1. run the command in a shell
2. attach the output buffer

[simark]

Sounds like a different problem: What I am trying to fix is that the output buffer does not get populated at all because the command has already finished before the output buffer is populated.

I don't understand what you mean here. The current code creates a tty (Theia owns the master side of it), then spawns a child whose stdint/stdout/stderr are the slave end of the tty. Even if the child writes data and exits quickly, the data should not disappear, it's buffered in the tty buffer. We can then read it and display it in the appropriate terminal window, even after the process has ended.

I'd very much like to see what happens exactly on your end. Does it look like the following?

[JanKoehnlein]

Yes, that's what I see. I faintly remember I also had longer running jobs whose first lines of output were swallowed, but I am not entirely sure anymore. But I get the same error message on the console as you. So if you have a good fix for that, go ahead.

If you look at the code of the TerminalProcess, the method spawn is already called with the command (not the shell). After that the output buffer is attached. I think, a race between the process and the output attachment can cause the loss of output lines. The proposed fix always opens an interactive shell, but doesn't loose any output (tested on Windows and Mac). Maybe there is more to it.

Furthermore, the documentation also suggests that on windows I should specify cmd.exe as the command, while on other OSs I did not have to specify any shell. The proposed change also fixes this asymmetry and brings us closer to the VSCode task format.

[simark]

Yes, that's what I see. I faintly remember I also had longer running jobs whose first lines of output were swallowed, but I am not entirely sure anymore. But I get the same error message on the console as you.

Can you please try to reproduce? I have a hard time seeing how this could happen, but if I had a way to reproduce it would help.

So if you have a good fix for that, go ahead.

I haven't looked into it yet, just thought about the possible paths mentioned above.

If you look at the code of the TerminalProcess, the method spawn is already called with the command (not the shell). After that the output buffer is attached. I think, a race between the process and the output attachment can cause the loss of output lines.

The way this is designed, it's not supposed to happen. It works the same way with node's child_process.spawn, you register a data callback after having created the process. Between the time you get the return value in this.terminal and the time you register the callback, the execution does not go back to the event loop, so there should be no way that the data output by the process would be consumed. If there is indeed a window where the data gets consumed before we have the change to register the callback, I'd say it's a bug with node-pty.

Furthermore, the documentation also suggests that on windows I should specify cmd.exe as the command, while on other OSs I did not have to specify any shell. The proposed change also fixes this asymmetry and brings us closer to the VSCode task format.

Which documentation?

I agree we are quite far from how VSCode handles and executes tasks (e.g. #2960) and it's also my goal to bring us closer.

[simark]

Ideally, our process API should be flexible enough to support these two axis of configuration independently:

• What to use as standard input/output: a tty, pipes or nothing
• Should the command be run through a shell, or directly as a process

[JanKoehnlein]

The documentation: README.md in the task extension's base dir.

When the command is run as a new process, does it really matter what the JS event loop does? I wouldn't claim the missing lines are consumed. I assume they are just missed. Of course I may be wrong. Sounds like you have dug deeper into the node-pty code. I won't even rule out it has bugs. At least the example in the README of node-pty does it the way I designed my fix.

I am open to any kind of solution to this. I applied my fix for a customer project and it worked well enough for them. It was much more important to see the full output (especially on errors) than to avoid an interactive terminal.