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

Fix output race condition in Process.exec #46

Merged
merged 1 commit into from
Jun 1, 2022
Merged

Fix output race condition in Process.exec #46

merged 1 commit into from
Jun 1, 2022

Conversation

kroppt
Copy link
Contributor

@kroppt kroppt commented Jun 1, 2022

I encountered an issue when starting projects up. Others have had the problem as well, so I fixed it in this pull request.

It has to do with the definition of Process.exec, which the Ionide extension uses to find the dotnet executable.

Per the documentation of child_process:

The 'close' event is emitted after a process has ended and the stdio streams of a child process have been closed. This is distinct from the 'exit' event, since multiple processes might share the same stdio streams. The 'close' event will always emit after 'exit' was already emitted, or 'error' if the child failed to spawn.

The 'exit' event is emitted after the child process ends. If the process exited, code is the final exit code of the process, otherwise null. If the process terminated due to receipt of a signal, signal is the string name of the signal, otherwise null. One of the two will always be non-null.

When the 'exit' event is triggered, child process stdio streams might still be open.

Thus, because the output of Process.exec relies on the accumulated output of the child process, the promise must relolve on the close event instead of the exit event.

Fixes ionide/ionide-vscode-fsharp#1712
Fixes ionide/ionide-vscode-fsharp#1686

@baronfel
Copy link
Contributor

baronfel commented Jun 1, 2022

Thank you for the detailed analysis, and the fix! I'll merge and get this out in the next day or two.

@baronfel baronfel merged commit 9782d18 into ionide:master Jun 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants