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

Add simple stdin support #130

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions source/Shellfish/IInputSource.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
using System.Collections.Generic;

namespace Octopus.Shellfish;

public interface IInputSource
{
IEnumerable<string> GetInput();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see what you're going for here.
My one basically using IObservable... IObservable is the mirror-image of IEnumerable so you end up with the same approach, just synchronous instead of async.

That said, while sync is much simpler, I'm not sure it's safe or correct 🤔 Will be fun to chat through it.

}
47 changes: 40 additions & 7 deletions source/Shellfish/ShellCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ public class ShellCommand

List<IOutputTarget>? stdOutTargets;
List<IOutputTarget>? stdErrTargets;
IInputSource? stdInSource;

public ShellCommand(string executable)
{
Expand Down Expand Up @@ -138,18 +139,24 @@ public ShellCommand WithStdErrTarget(IOutputTarget target)
return this;
}

public ShellCommand WithStdInSource(IInputSource source)
{
stdInSource = source;
return this;
}

/// <summary>
/// Launches the process and synchronously waits for it to exit.
/// </summary>
public ShellCommandResult Execute(CancellationToken cancellationToken = default)
{
using var process = new Process();
ConfigureProcess(process, out var shouldBeginOutputRead, out var shouldBeginErrorRead);
ConfigureProcess(process, out var shouldBeginOutputRead, out var shouldBeginErrorRead, out var redirectingStdIn);

var exitedEvent = AttachProcessExitedManualResetEvent(process, cancellationToken);
process.Start();

BeginIoStreams(process, shouldBeginOutputRead, shouldBeginErrorRead);
BeginIoStreams(process, shouldBeginOutputRead, shouldBeginErrorRead, redirectingStdIn);

try
{
Expand Down Expand Up @@ -185,12 +192,12 @@ public ShellCommandResult Execute(CancellationToken cancellationToken = default)
public async Task<ShellCommandResult> ExecuteAsync(CancellationToken cancellationToken = default)
{
using var process = new Process();
ConfigureProcess(process, out var shouldBeginOutputRead, out var shouldBeginErrorRead);
ConfigureProcess(process, out var shouldBeginOutputRead, out var shouldBeginErrorRead, out var redirectingStdIn);

var exitedTask = AttachProcessExitedTask(process, cancellationToken);
process.Start();

BeginIoStreams(process, shouldBeginOutputRead, shouldBeginErrorRead);
BeginIoStreams(process, shouldBeginOutputRead, shouldBeginErrorRead, redirectingStdIn);

try
{
Expand All @@ -216,7 +223,7 @@ public async Task<ShellCommandResult> ExecuteAsync(CancellationToken cancellatio
}

// sets standard flags on the Process that apply for both Execute and ExecuteAsync
void ConfigureProcess(Process process, out bool shouldBeginOutputRead, out bool shouldBeginErrorRead)
void ConfigureProcess(Process process, out bool shouldBeginOutputRead, out bool shouldBeginErrorRead, out bool redirectingStdIn)
{
process.StartInfo.FileName = executable;

Expand Down Expand Up @@ -264,7 +271,7 @@ void ConfigureProcess(Process process, out bool shouldBeginOutputRead, out bool
}
}

shouldBeginOutputRead = shouldBeginErrorRead = false;
shouldBeginOutputRead = shouldBeginErrorRead = redirectingStdIn = false;
if (stdOutTargets is { Count: > 0 })
{
process.StartInfo.RedirectStandardOutput = true;
Expand Down Expand Up @@ -292,15 +299,41 @@ void ConfigureProcess(Process process, out bool shouldBeginOutputRead, out bool
foreach (var target in targets) target.WriteLine(e.Data);
};
}

if (stdInSource is not null)
{
process.StartInfo.RedirectStandardInput = true;
redirectingStdIn = true;
}
}

// Common code for Execute and ExecuteAsync to handle stdin and stdout streaming
void BeginIoStreams(Process process,
bool shouldBeginOutputRead,
bool shouldBeginErrorRead)
bool shouldBeginErrorRead,
bool redirectingStdIn)
{
if (shouldBeginOutputRead) process.BeginOutputReadLine();
if (shouldBeginErrorRead) process.BeginErrorReadLine();

if (redirectingStdIn)
{
try
{
foreach (var val in stdInSource!.GetInput())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the first iteration of my stdin one, I effectively wrote this; I just had an array of strings for stdin, and I wrote them all in a foreach loop exactly like this one.

I found though, that when I ran the test that asks for more than one line of input, it didn't work. What appeared to happen was that the first line of text would get sent to the first prompt, the second line of text would get dropped because the shell script hadn't yet asked for any input, and then shortly later when it did ask, it was too late.

I see you've brought that test across in this PR. Does it work for you? If so my hypothesis above must have been wrong in some way.

Anyway, while this should work well for simple programs, there are two things which give me pause

  1. We are single-threaded in the path of launching the process. If someone were to set up a stdin string for a program that wasn't expecting it, it might never read from stdin and our write would block forever. Our whole code-path would stall.
    1. Related to that: If our input enumerable takes a long time to enumerate, we'd also stall
    2. This approach works for small data sizes only because C# Streams have inbuilt buffering. If you exceed the buffer size, then writes will block until the other end starts to consume the data. Stall
  2. As I discovered, some programs exit when stdin is closed. octopus.server run is such a thing, and ross needed to pass in a dummy stdin stream that never closes, to stop it shutting itself down immediately. With an IEnumerable-based stream like this, how could we pull that off?

{
process.StandardInput.Write(val);
}
}
catch (OperationCanceledException)
{
// gracefully handle cancellation of the enumerator
}
finally
{
process.StandardInput.Close();
}
}
}

static async Task FinalWaitForExitAsync(Process process, CancellationToken cancellationToken)
Expand Down
17 changes: 17 additions & 0 deletions source/Shellfish/StringInputSource.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
using System.Collections.Generic;

namespace Octopus.Shellfish;

class StringInputSource(string value) : IInputSource
{
public IEnumerable<string> GetInput() => [value];
}

public static class StringInputSourceExtensions
{
public static ShellCommand WithStdInSource(this ShellCommand shellCommand, string input)
{
shellCommand.WithStdInSource(new StringInputSource(input));
return shellCommand;
}
}
61 changes: 61 additions & 0 deletions source/Tests/Plumbing/TempScript.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
using System;
using System.IO;
using System.Runtime.InteropServices;

namespace Tests.Plumbing;

public static class TempScript
{
// Some interactions such as stdout or encoding codepages require things that don't work with an inline cmd /c or bash -c command
// This helper writes a script file into the temp directory so we can exercise more complex scenarios
public static Handle Create(string cmd, string sh)
{
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
{
var tempFile = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N") + ".cmd");
File.WriteAllText(tempFile, cmd);
return new Handle(tempFile);
}
else
{
var tempFile = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N") + ".sh");
File.WriteAllText(tempFile, sh.Replace("\r\n", "\n"));
return new Handle(tempFile);
}
}

public class Handle(string scriptPath) : IDisposable
{
public string ScriptPath { get; } = scriptPath;

public void Dispose()
{
try
{
File.Delete(ScriptPath);
}
catch
{
// nothing to do if we can't delete the temp file
}
}

// Returns the host application which will run the script. Either cmd.exe or bash
public string GetHostExecutable()
{
return RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "cmd.exe" : "bash";
}

// Returns the command line args to get the host application to run the script
// For cmd.exe, returns ["/c", ScriptPath] as it needs /c
// For bash, returns [ScriptPath] as it doesn't need any preamble
public string[] GetCommandArgs()
{
// when running cmd.exe we need /c to tell it to run the script; bash doesn't want any preamble for a script file
return RuntimeInformation.IsOSPlatform(OSPlatform.Windows)
? ["/c", ScriptPath]
: [ScriptPath];

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ Octopus.Shellfish.ShellCommandExtensionMethods.WithStdOutTarget
Octopus.Shellfish.ShellCommandExtensionMethods.WithStdErrTarget
Octopus.Shellfish.ShellCommandExtensionMethods.WithStdOutTarget
Octopus.Shellfish.ShellCommandExtensionMethods.WithStdErrTarget
Octopus.Shellfish.IInputSource.GetInput
Octopus.Shellfish.IOutputTarget.WriteLine
Octopus.Shellfish.ShellCommand.WithWorkingDirectory
Octopus.Shellfish.ShellCommand.WithArguments
Expand All @@ -11,10 +12,12 @@ Octopus.Shellfish.ShellCommand.WithCredentials
Octopus.Shellfish.ShellCommand.WithOutputEncoding
Octopus.Shellfish.ShellCommand.WithStdOutTarget
Octopus.Shellfish.ShellCommand.WithStdErrTarget
Octopus.Shellfish.ShellCommand.WithStdInSource
Octopus.Shellfish.ShellCommand.Execute
Octopus.Shellfish.ShellCommand.ExecuteAsync
Octopus.Shellfish.ShellCommandResult.ExitCode
Octopus.Shellfish.ShellExecutionException.Errors
Octopus.Shellfish.ShellExecutionException.Message
Octopus.Shellfish.ShellExecutor.ExecuteCommand
Octopus.Shellfish.ShellExecutor.ExecuteCommandWithoutWaiting
Octopus.Shellfish.ShellExecutor.ExecuteCommandWithoutWaiting
Octopus.Shellfish.StringInputSourceExtensions.WithStdInSource
Loading