Merge pull request #1240 from dotnet/brthor/stream_forwarding_changes

Stream Forwarding Changes.

Documentation/developer-guide.md

@@ -65,3 +65,6 @@ Each command's project root should contain a manpage-style Readme.md that descri
 #### Add command to packages
 - Update the `symlinks` property of `packaging/debian/debian_config.json` to include the new command
 - Update the `$Projects` property in `packaging/osx/scripts/postinstall`
+
+#### Things to Know
+- Any added commands are usually invoked through `dotnet {command}`. As a result of this, stdout and stderr are redirected through the driver (`dotnet`) and buffered by line. As a result of this, child commands should use Console.WriteLine in any cases where they expect output to be written immediately. Any uses of Console.Write should be followed by Console.WriteLine to ensure the output is written.

src/Microsoft.DotNet.Cli.Utils/Command.cs

@@ -111,8 +111,8 @@ namespace Microsoft.DotNet.Cli.Utils
             return new CommandResult(
                 this._process.StartInfo,
                 exitCode,
-                _stdOut.GetCapturedOutput(),
+                _stdOut.CapturedOutput,
-                _stdErr.GetCapturedOutput());
+                _stdErr.CapturedOutput);
         }
 
         public Command WorkingDirectory(string projectDirectory)
@@ -148,11 +148,11 @@ namespace Microsoft.DotNet.Cli.Utils
             {
                 if (to == null)
                 {
-                    _stdOut.ForwardTo(write: Reporter.Output.Write, writeLine: Reporter.Output.WriteLine);
+                    _stdOut.ForwardTo(writeLine: Reporter.Output.WriteLine);
                 }
                 else
                 {
-                    _stdOut.ForwardTo(write: to.Write, writeLine: to.WriteLine);
+                    _stdOut.ForwardTo(writeLine: to.WriteLine);
                 }
             }
             return this;
@@ -165,11 +165,11 @@ namespace Microsoft.DotNet.Cli.Utils
             {
                 if (to == null)
                 {
-                    _stdErr.ForwardTo(write: Reporter.Error.Write, writeLine: Reporter.Error.WriteLine);
+                    _stdErr.ForwardTo(writeLine: Reporter.Error.WriteLine);
                 }
                 else
                 {
-                    _stdErr.ForwardTo(write: to.Write, writeLine: to.WriteLine);
+                    _stdErr.ForwardTo(writeLine: to.WriteLine);
                 }
             }
             return this;
@@ -178,14 +178,14 @@ namespace Microsoft.DotNet.Cli.Utils
         public Command OnOutputLine(Action<string> handler)
         {
             ThrowIfRunning();
-            _stdOut.ForwardTo(write: null, writeLine: handler);
+            _stdOut.ForwardTo(writeLine: handler);
             return this;
         }
 
         public Command OnErrorLine(Action<string> handler)
         {
             ThrowIfRunning();
-            _stdErr.ForwardTo(write: null, writeLine: handler);
+            _stdErr.ForwardTo(writeLine: handler);
             return this;
         }
 

src/Microsoft.DotNet.Cli.Utils/Reporter.cs

@@ -9,7 +9,7 @@ namespace Microsoft.DotNet.Cli.Utils
     // Stupid-simple console manager
     public class Reporter
     {
-        private static readonly Reporter Null = new Reporter(console: null);
+        private static readonly Reporter NullReporter = new Reporter(console: null);
         private static object _lock = new object();
 
         private readonly AnsiConsole _console;
@@ -20,8 +20,8 @@ namespace Microsoft.DotNet.Cli.Utils
         }
 
         public static Reporter Output { get; } = Create(AnsiConsole.GetOutput);
-        public static Reporter Error { get; } = Create(AnsiConsole.GetOutput);
+        public static Reporter Error { get; } = Create(AnsiConsole.GetError);
-        public static Reporter Verbose { get; } = CommandContext.IsVerbose() ? Create(AnsiConsole.GetOutput) : Null;
+        public static Reporter Verbose { get; } = CommandContext.IsVerbose() ? Create(AnsiConsole.GetOutput) : NullReporter;
 
         public static Reporter Create(Func<bool, AnsiConsole> getter)
         {

src/Microsoft.DotNet.Cli.Utils/StreamForwarder.cs

@@ -2,50 +2,46 @@ using System;
 using System.IO;
 using System.Text;
 using System.Threading;
+using System.Linq;
 
 namespace Microsoft.DotNet.Cli.Utils
 {
     public sealed class StreamForwarder
     {
-        private const int DefaultBufferSize = 256;
+        private static readonly char[] s_ignoreCharacters = new char[] { '\r' };
+        private static readonly char s_flushBuilderCharacter = '\n';
 
-        private readonly int _bufferSize;
         private StringBuilder _builder;
         private StringWriter _capture;
         private Action<string> _write;
         private Action<string> _writeLine;
 
-        public StreamForwarder(int bufferSize = DefaultBufferSize)
+        public string CapturedOutput
         {
-            _bufferSize = bufferSize;
+            get 
+            {
+                return _capture?.GetStringBuilder()?.ToString();
+            }
         }
 
-        public void Capture()
+        public StreamForwarder Capture()
         {
-            if (_capture != null)
+            ThrowIfCaptureSet();
-            {
+
-                throw new InvalidOperationException("Already capturing stream!");
-            }
             _capture = new StringWriter();
+
+            return this;
         }
 
-        public string GetCapturedOutput()
+        public StreamForwarder ForwardTo(Action<string> writeLine)
         {
-            return _capture?.GetStringBuilder()?.ToString();
+            ThrowIfNull(writeLine);
-        }
+
+            ThrowIfForwarderSet();
 
-        public void ForwardTo(Action<string> write, Action<string> writeLine)
-        {
-            if (writeLine == null)
-            {
-                throw new ArgumentNullException(nameof(writeLine));
-            }
-            if (_writeLine != null)
-            {
-                throw new InvalidOperationException("Already handling stream!");
-            }
-            _write = write;
             _writeLine = writeLine;
+
+            return this;
         }
 
         public Thread BeginRead(TextReader reader)
@@ -57,93 +53,80 @@ namespace Microsoft.DotNet.Cli.Utils
 
         public void Read(TextReader reader)
         {
+            var bufferSize = 1;
+
+            int readCharacterCount; 
+            char currentCharacter;
+
+            var buffer = new char[bufferSize];
             _builder = new StringBuilder();
-            var buffer = new char[_bufferSize];
-            int n;
-            while ((n = reader.Read(buffer, 0, _bufferSize)) > 0)
-            {
-                _builder.Append(buffer, 0, n);
-                WriteBlocks();
-            }
-            WriteRemainder();
-        }
 
-        private void WriteBlocks()
+            // Using Read with buffer size 1 to prevent looping endlessly
-        {
+            // like we would when using Read() with no buffer
-            int n = _builder.Length;
+            while ((readCharacterCount = reader.Read(buffer, 0, bufferSize)) > 0)
-            if (n == 0)
             {
-                return;
+                currentCharacter = buffer[0];
-            }
 
-            int offset = 0;
+                if (currentCharacter == s_flushBuilderCharacter)
-            bool sawReturn = false;
-            for (int i = 0; i < n; i++)
-            {
-                char c = _builder[i];
-                switch (c)
                 {
-                    case '\r':
+                    WriteBuilder();
-                        sawReturn = true;
+                }
-                        continue;
+                else if (! s_ignoreCharacters.Contains(currentCharacter))
-                    case '\n':
+                {
-                        WriteLine(_builder.ToString(offset, i - offset - (sawReturn ? 1 : 0)));
+                    _builder.Append(currentCharacter);
-                        offset = i + 1;
-                        break;
                 }
-                sawReturn = false;
             }
 
-            // If the buffer contains no line breaks and _write is
+            // Flush anything else when the stream is closed
-            // supported, send the buffer content.
+            // Which should only happen if someone used console.Write
-            if (!sawReturn &&
+            WriteBuilder();
-                (offset == 0) &&
-                ((_write != null) || (_writeLine == null)))
-            {
-                WriteRemainder();
-            }
-            else
-            {
-                _builder.Remove(0, offset);
-            }
         }
 
-        private void WriteRemainder()
+        private void WriteBuilder()
         {
             if (_builder.Length == 0)
             {
                 return;
             }
-            Write(_builder.ToString());
+
+            WriteLine(_builder.ToString());
             _builder.Clear();
         }
 
         private void WriteLine(string str)
-        {
+        { 
             if (_capture != null)
             {
                 _capture.WriteLine(str);
             }
-            // If _write is supported, so is _writeLine.
+
             if (_writeLine != null)
             {
                 _writeLine(str);
             }
         }
 
-        private void Write(string str)
+        private void ThrowIfNull(object obj)
+        {
+            if (obj == null)
+            {
+                throw new ArgumentNullException(nameof(obj));
+            }
+        }
+
+        private void ThrowIfForwarderSet()
+        {
+            if (_writeLine != null)
+            {
+                throw new InvalidOperationException("WriteLine forwarder set previously");
+            }
+        }
+
+        private void ThrowIfCaptureSet()
         {
             if (_capture != null)
             {
-                _capture.Write(str);
+                throw new InvalidOperationException("Already capturing stream!");
-            }
-            if (_write != null)
-            {
-                _write(str);
-            }
-            else if (_writeLine != null)
-            {
-                _writeLine(str);
             }
         }
     }

src/dotnet/commands/dotnet-restore/Program.cs

@@ -21,7 +21,6 @@ namespace Microsoft.DotNet.Tools.Restore
     {
         private static readonly string DefaultRid = PlatformServices.Default.Runtime.GetLegacyRestoreRuntimeIdentifier();
 
-
         public static int Run(string[] args)
         {
             DebugHelper.HandleDebugSwitch(ref args);

test/ArgumentForwardingTests/ArgumentForwardingTests/ArgumentForwardingTests.cs

@@ -206,7 +206,7 @@ namespace Microsoft.DotNet.Tests.ArgumentForwarding
         /// <returns></returns>
         private string[] ParseReflectorOutput(string reflectorOutput)
         {
-            return reflectorOutput.Split(',');
+            return reflectorOutput.TrimEnd('\r', '\n').Split(',');
         }
 
         /// <summary>

test/Microsoft.DotNet.Tools.Tests.Utilities/Commands/TestCommand.cs

@@ -32,8 +32,8 @@ namespace Microsoft.DotNet.Tools.Test.Utilities
             var stdOut = new StreamForwarder();
             var stdErr = new StreamForwarder();
 
-            stdOut.ForwardTo(write: Reporter.Output.Write, writeLine: Reporter.Output.WriteLine);
+            stdOut.ForwardTo(writeLine: Reporter.Output.WriteLine);
-            stdErr.ForwardTo(write: Reporter.Error.Write, writeLine: Reporter.Output.WriteLine);
+            stdErr.ForwardTo(writeLine: Reporter.Output.WriteLine);
 
             return RunProcess(commandPath, args, stdOut, stdErr);
         }
@@ -82,8 +82,8 @@ namespace Microsoft.DotNet.Tools.Test.Utilities
             var result = new CommandResult(
                 process.StartInfo,
                 process.ExitCode, 
-                stdOut.GetCapturedOutput(), 
+                stdOut.CapturedOutput, 
-                stdErr.GetCapturedOutput());
+                stdErr.CapturedOutput);
 
             return result;
         }

test/StreamForwarderTests/StreamForwarderTests.cs

@@ -10,109 +10,138 @@ using Xunit;
 using Microsoft.DotNet.Cli.Utils;
 using Microsoft.DotNet.ProjectModel;
 using Microsoft.DotNet.Tools.Test.Utilities;
+using Microsoft.Extensions.PlatformAbstractions;
+using System.Threading;
 
 namespace StreamForwarderTests
 {
-    public class StreamForwarderTests
+    public class StreamForwarderTests : TestBase
     {
+        private static readonly string s_rid = PlatformServices.Default.Runtime.GetLegacyRestoreRuntimeIdentifier();
+        private static readonly string s_testProjectRoot = Path.Combine(AppContext.BaseDirectory, "TestProjects");
+
+        private TempDirectory _root;
+
         public static void Main()
         {
             Console.WriteLine("Dummy Entrypoint");
         }
 
-        [Fact]
+        public StreamForwarderTests()
-        public void Unbuffered()
         {
-            Forward(4, true, "");
+            _root = Temp.CreateDirectory();
-            Forward(4, true, "123", "123");
-            Forward(4, true, "1234", "1234");
-            Forward(3, true, "123456789", "123", "456", "789");
-            Forward(4, true, "\r\n", "\n");
-            Forward(4, true, "\r\n34", "\n", "34");
-            Forward(4, true, "1\r\n4", "1\n", "4");
-            Forward(4, true, "12\r\n", "12\n");
-            Forward(4, true, "123\r\n", "123\n");
-            Forward(4, true, "1234\r\n", "1234", "\n");
-            Forward(3, true, "\r\n3456\r\n9", "\n", "3456", "\n", "9");
-            Forward(4, true, "\n", "\n");
-            Forward(4, true, "\n234", "\n", "234");
-            Forward(4, true, "1\n34", "1\n", "34");
-            Forward(4, true, "12\n4", "12\n", "4");
-            Forward(4, true, "123\n", "123\n");
-            Forward(4, true, "1234\n", "1234", "\n");
-            Forward(3, true, "\n23456\n89", "\n", "23456", "\n", "89");
         }
 
-        [Fact]
+        public static IEnumerable<object[]> ForwardingTheoryVariations
-        public void LineBuffered()
         {
-            Forward(4, false, "");
+            get
-            Forward(4, false, "123", "123\n");
+            {
-            Forward(4, false, "1234", "1234\n");
+                return new[]
-            Forward(3, false, "123456789", "123456789\n");
+                {
-            Forward(4, false, "\r\n", "\n");
+                    new object[] { "123", new string[]{"123"} },
-            Forward(4, false, "\r\n34", "\n", "34\n");
+                    new object[] { "123\n", new string[] {"123"} },
-            Forward(4, false, "1\r\n4", "1\n", "4\n");
+                    new object[] { "123\r\n", new string[] {"123"} },
-            Forward(4, false, "12\r\n", "12\n");
+                    new object[] { "1234\n5678", new string[] {"1234", "5678"} },
-            Forward(4, false, "123\r\n", "123\n");
+                    new object[] { "1234\r\n5678", new string[] {"1234", "5678"} },
-            Forward(4, false, "1234\r\n", "1234\n");
+                    new object[] { "1234\n5678\n", new string[] {"1234", "5678"} },
-            Forward(3, false, "\r\n3456\r\n9", "\n", "3456\n", "9\n");
+                    new object[] { "1234\r\n5678\r\n", new string[] {"1234", "5678"} },
-            Forward(4, false, "\n", "\n");
+                    new object[] { "1234\n5678\nabcdefghijklmnopqrstuvwxyz", new string[] {"1234", "5678", "abcdefghijklmnopqrstuvwxyz"} },
-            Forward(4, false, "\n234", "\n", "234\n");
+                    new object[] { "1234\r\n5678\r\nabcdefghijklmnopqrstuvwxyz", new string[] {"1234", "5678", "abcdefghijklmnopqrstuvwxyz"} },
-            Forward(4, false, "1\n34", "1\n", "34\n");
+                    new object[] { "1234\n5678\nabcdefghijklmnopqrstuvwxyz\n", new string[] {"1234", "5678", "abcdefghijklmnopqrstuvwxyz"} },
-            Forward(4, false, "12\n4", "12\n", "4\n");
+                    new object[] { "1234\r\n5678\r\nabcdefghijklmnopqrstuvwxyz\r\n", new string[] {"1234", "5678", "abcdefghijklmnopqrstuvwxyz"} }
-            Forward(4, false, "123\n", "123\n");
+                };
-            Forward(4, false, "1234\n", "1234\n");
+            }
-            Forward(3, false, "\n23456\n89", "\n", "23456\n", "89\n");
         }
 
-        private static void Forward(int bufferSize, bool unbuffered, string str, params string[] expectedWrites)
+        [Theory]
+        [InlineData("123")]
+        [InlineData("123\n")]
+        public void TestNoForwardingNoCapture(string inputStr)
+        {   
+            TestCapturingAndForwardingHelper(ForwardOptions.None, inputStr, null, new string[0]);
+        }
+
+        [Theory]
+        [MemberData("ForwardingTheoryVariations")]
+        public void TestForwardingOnly(string inputStr, string[] expectedWrites)
         {
-            var expectedCaptured = str.Replace("\r", "").Replace("\n", Environment.NewLine);
+            for(int i = 0; i < expectedWrites.Length; ++i)
+            {
+                expectedWrites[i] += Environment.NewLine;
+            }
+            
+            TestCapturingAndForwardingHelper(ForwardOptions.WriteLine, inputStr, null, expectedWrites);
+        }
 
-            // No forwarding.
+        [Theory]
-            Forward(bufferSize, ForwardOptions.None, str, null, new string[0]);
+        [MemberData("ForwardingTheoryVariations")]
+        public void TestCaptureOnly(string inputStr, string[] expectedWrites)
+        {
+            for(int i = 0; i < expectedWrites.Length; ++i)
+            {
+                expectedWrites[i] += Environment.NewLine;
+            }
 
-            // Capture only.
+            var expectedCaptured = string.Join("", expectedWrites);
-            Forward(bufferSize, ForwardOptions.Capture, str, expectedCaptured, new string[0]);
+            
+            TestCapturingAndForwardingHelper(ForwardOptions.Capture, inputStr, expectedCaptured, new string[0]);
+        }
 
-            var writeOptions = unbuffered ?
+        [Theory]
-                ForwardOptions.Write | ForwardOptions.WriteLine :
+        [MemberData("ForwardingTheoryVariations")]
-                ForwardOptions.WriteLine;
+        public void TestCaptureAndForwardingTogether(string inputStr, string[] expectedWrites)
+        {
+            for(int i = 0; i < expectedWrites.Length; ++i)
+            {
+                expectedWrites[i] += Environment.NewLine;
+            }
 
-            // Forward.
+            var expectedCaptured = string.Join("", expectedWrites);
-            Forward(bufferSize, writeOptions, str, null, expectedWrites);
 
-            // Forward and capture.
+            TestCapturingAndForwardingHelper(ForwardOptions.WriteLine | ForwardOptions.Capture, inputStr, expectedCaptured, expectedWrites);
-            Forward(bufferSize, writeOptions | ForwardOptions.Capture, str, expectedCaptured, expectedWrites);
         }
 
         private enum ForwardOptions
         {
             None = 0x0,
             Capture = 0x1,
-            Write = 0x02,
+            WriteLine = 0x02,
-            WriteLine = 0x04,
         }
 
-        private static void Forward(int bufferSize, ForwardOptions options, string str, string expectedCaptured, string[] expectedWrites)
+        private void TestCapturingAndForwardingHelper(ForwardOptions options, string str, string expectedCaptured, string[] expectedWrites)
         {
-            var forwarder = new StreamForwarder(bufferSize);
+            var forwarder = new StreamForwarder();
             var writes = new List<string>();
+
             if ((options & ForwardOptions.WriteLine) != 0)
             {
-                forwarder.ForwardTo(
+                forwarder.ForwardTo(writeLine: s => writes.Add(s + Environment.NewLine));
-                    write: (options & ForwardOptions.Write) == 0 ? (Action<string>)null : writes.Add,
-                    writeLine: s => writes.Add(s + "\n"));
             }
             if ((options & ForwardOptions.Capture) != 0)
             {
                 forwarder.Capture();
             }
+
             forwarder.Read(new StringReader(str));
             Assert.Equal(expectedWrites, writes);
-            var captured = forwarder.GetCapturedOutput();
+
+            var captured = forwarder.CapturedOutput;
             Assert.Equal(expectedCaptured, captured);
         }
+
+        private string SetupTestProject()
+        {
+            var sourceTestProjectPath = Path.Combine(s_testProjectRoot, "OutputStandardOutputAndError");
+
+            var binTestProjectPath = _root.CopyDirectory(sourceTestProjectPath).Path;
+
+            var buildCommand = new BuildCommand(Path.Combine(binTestProjectPath, "project.json"));
+            buildCommand.Execute();
+
+            var buildOutputExe = "OutputStandardOutputAndError" + Constants.ExeSuffix;
+            var buildOutputPath = Path.Combine(binTestProjectPath, "bin/Debug/dnxcore50", buildOutputExe);
+
+            return buildOutputPath;
+        }
     }
 }

test/StreamForwarderTests/project.json

@@ -22,5 +22,9 @@
         }
     },
 
+    "content": [
+        "../TestProjects/OutputStandardOutputAndError/*"
+    ],
+
     "testRunner": "xunit"
 }

test/TestProjects/OutputStandardOutputAndError/OutputStandardOutputAndError.xproj

@@ -0,0 +1,20 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="14.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <PropertyGroup>
+    <VisualStudioVersion Condition="'$(VisualStudioVersion)' == ''">14.0</VisualStudioVersion>
+    <VSToolsPath Condition="'$(VSToolsPath)' == ''">$(MSBuildExtensionsPath32)\Microsoft\VisualStudio\v$(VisualStudioVersion)</VSToolsPath>
+  </PropertyGroup>
+
+  <Import Project="$(VSToolsPath)\DNX\Microsoft.DNX.Props" Condition="'$(VSToolsPath)' != ''" />
+  <PropertyGroup Label="Globals">
+    <ProjectGuid>58808bbc-371e-47d6-a3d0-4902145eda4e</ProjectGuid>
+    <RootNamespace>OutputStandardOutputAndError</RootNamespace>
+    <BaseIntermediateOutputPath Condition="'$(BaseIntermediateOutputPath)'=='' ">..\..\artifacts\obj\$(MSBuildProjectName)</BaseIntermediateOutputPath>
+    <OutputPath Condition="'$(OutputPath)'=='' ">..\..\artifacts\bin\$(MSBuildProjectName)\</OutputPath>
+  </PropertyGroup>
+
+  <PropertyGroup>
+    <SchemaVersion>2.0</SchemaVersion>
+  </PropertyGroup>
+  <Import Project="$(VSToolsPath)\DNX\Microsoft.DNX.targets" Condition="'$(VSToolsPath)' != ''" />
+</Project>

test/TestProjects/OutputStandardOutputAndError/Program.cs

@@ -0,0 +1,19 @@
+// Copyright (c) .NET Foundation and contributors. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using System;
+using System.Diagnostics;
+
+namespace TestApp
+{
+    public class Program
+    {
+        public static void Main(string[] args)
+        {
+            Console.Out.WriteLine("** Standard Out 1 **");
+            Console.Error.WriteLine("** Standard Error 1 **");
+            Console.Out.WriteLine("** Standard Out 2 **");
+            Console.Error.WriteLine("** Standard Error 2 **");
+        }
+    }
+}

test/TestProjects/OutputStandardOutputAndError/project.json

@@ -0,0 +1,14 @@
+{
+    "version": "1.0.0-*",
+    "compilationOptions": {
+        "emitEntryPoint": true
+    },
+
+    "dependencies": {
+        "NETStandard.Library": "1.0.0-rc2-23728"
+    },
+
+    "frameworks": {
+        "dnxcore50": { }
+    }
+}

test/dotnet-build.Tests/IncrementalTests.cs

@@ -62,7 +62,7 @@ namespace Microsoft.DotNet.Tools.Builder.Tests
             Assert.False(File.Exists(lockFile));
 
             buildResult = BuildProject(expectBuildFailure : true);
-            Assert.Contains("does not have a lock file", buildResult.StdOut);
+            Assert.Contains("does not have a lock file", buildResult.StdErr);
         }
 
         [Fact]

test/dotnet-compile.Tests/CompilerTests.cs

@@ -58,7 +58,8 @@ namespace Microsoft.DotNet.Tools.Compiler.Tests
             var buildCmd = new BuildCommand(testProject, output: outputDir, framework: DefaultFramework);
             var result = buildCmd.ExecuteWithCapturedOutput();
             result.Should().Pass();
-            Assert.Contains("CA1018", result.StdOut);
+            
+            Assert.Contains("CA1018", result.StdErr);
         }
 
         private void CopyProjectToTempDir(string projectDir, TempDirectory tempDir)