Keep a surrogate pair in one output chunk [patch]

RunCommand.Test/AsyncProcessStreamReaderTests.cs

@@ -0,0 +1,71 @@
+// Copyright (c) 2023-2026 ktsu-dev contributors
+
+namespace ktsu.RunCommand.Test;
+
+[TestClass]
+public class AsyncProcessStreamReaderTests
+{
+	private const string Emoji = "\U0001F600";
+	private static readonly string[] ExpectedOneCharChunks = ["x", Emoji, Emoji, "y", Emoji];
+	private static readonly string[] ExpectedPlainChunks = ["abcd", "ef"];
+
+	private static async Task<List<string>> ReadChunksAsync(string text, int bufferSize)
+	{
+		List<string> chunks = [];
+		using StringReader reader = new(text);
+		await AsyncProcessStreamReader.ReadChunks(reader, new char[bufferSize], chunks.Add).ConfigureAwait(false);
+		return chunks;
+	}
+
+	private static void AssertNoChunkSplitsASurrogatePair(List<string> chunks)
+	{
+		for (int i = 0; i < chunks.Count; i++)
+		{
+			Assert.IsFalse(char.IsHighSurrogate(chunks[i][^1]), $"Chunk {i} ends with a lone high surrogate.");
+			Assert.IsFalse(char.IsLowSurrogate(chunks[i][0]), $"Chunk {i} starts with a lone low surrogate.");
+		}
+	}
+
+	[TestMethod]
+	public async Task ASurrogatePairOnABufferBoundaryIsDeliveredInOneChunk()
+	{
+		// The issue's reproduction: 4095 chars fill all but the last slot of the 4096-char buffer, so
+		// the read stops between the emoji's two halves.
+		string text = new string('a', 4095) + Emoji + new string('b', 5000);
+
+		List<string> chunks = await ReadChunksAsync(text, 4096).ConfigureAwait(false);
+
+		AssertNoChunkSplitsASurrogatePair(chunks);
+		Assert.AreEqual(text, string.Concat(chunks));
+	}
+
+	[TestMethod]
+	public async Task EverySurrogatePairSurvivesAOneCharBuffer()
+	{
+		string text = $"x{Emoji}{Emoji}y{Emoji}";
+
+		List<string> chunks = await ReadChunksAsync(text, 1).ConfigureAwait(false);
+
+		AssertNoChunkSplitsASurrogatePair(chunks);
+		CollectionAssert.AreEqual(ExpectedOneCharChunks, chunks);
+	}
+
+	[TestMethod]
+	public async Task AHighSurrogateAtEndOfStreamIsStillDelivered()
+	{
+		// Nothing follows it, so holding it back must not lose it.
+		string text = "ab\uD83D";
+
+		List<string> chunks = await ReadChunksAsync(text, 4096).ConfigureAwait(false);
+
+		Assert.AreEqual(text, string.Concat(chunks));
+	}
+
+	[TestMethod]
+	public async Task OutputWithoutSurrogatesIsDeliveredAsItIsRead()
+	{
+		List<string> chunks = await ReadChunksAsync("abcdef", 4).ConfigureAwait(false);
+
+		CollectionAssert.AreEqual(ExpectedPlainChunks, chunks);
+	}
+}

RunCommand/AsyncProcessStreamReader.cs

@@ -66,7 +66,7 @@
 		TaskCompletionSource<bool> cancellationSource = new(TaskCreationOptions.RunContinuationsAsynchronously);
 
 		using CancellationTokenRegistration registration = cancellationToken.Register(
 			static state => ((TaskCompletionSource<bool>)state!).TrySetResult(true),
 			cancellationSource);
 
 		Task cancelled = cancellationSource.Task;
@@ -149,13 +149,10 @@
 	/// <param name="buffer">The buffer each read fills.</param>
 	/// <param name="onData">The handler each chunk is passed to.</param>
 	/// <param name="isStandardOutput">Whether the stream is standard output.</param>
-	private async Task ReadToEnd(StreamReader streamReader, char[] buffer, Action<string>? onData, bool isStandardOutput)
-	{
-		int charsRead;
-		while ((charsRead = await streamReader.ReadAsync(buffer, 0, buffer.Length).ConfigureAwait(false)) > 0)
+	private Task ReadToEnd(StreamReader streamReader, char[] buffer, Action<string>? onData, bool isStandardOutput) =>
+		ReadChunks(streamReader, buffer, chunk =>
 		{
-			string data = new(buffer, 0, charsRead);
-			data = StripLeadingByteOrderMark(data, isStandardOutput);
+			string data = StripLeadingByteOrderMark(chunk, isStandardOutput);
 
 			if (data.Length > 0 && onData is not null)
 			{
@@ -164,6 +161,42 @@
 					onData(data);
 				}
 			}
+		});
+
+	/// <summary>
+	/// Reads <paramref name="reader"/> until end of stream, handing each read to
+	/// <paramref name="onChunk"/> without ever splitting a surrogate pair between two chunks.
+	/// </summary>
+	/// <remarks>
+	/// A read stops wherever the buffer fills, which can be between the two halves of a surrogate
+	/// pair. Passed on as it was, one chunk ended with a lone high surrogate and the next began with
+	/// a lone low surrogate, so a handler that encoded or serialized each chunk on its own wrote two
+	/// replacement characters instead of the one character the command printed. A trailing high
+	/// surrogate is therefore held back and put in front of the next read, and handed over on its own
+	/// only when the stream ends without the low half it was waiting for.
+	/// </remarks>
+	/// <param name="reader">The reader to drain.</param>
+	/// <param name="buffer">The buffer each read fills.</param>
+	/// <param name="onChunk">The callback each non-empty chunk is passed to.</param>
+	internal static async Task ReadChunks(TextReader reader, char[] buffer, Action<string> onChunk)
+	{
+		string heldBack = string.Empty;
+		int charsRead;
+		while ((charsRead = await reader.ReadAsync(buffer, 0, buffer.Length).ConfigureAwait(false)) > 0)
+		{
+			int length = char.IsHighSurrogate(buffer[charsRead - 1]) ? charsRead - 1 : charsRead;
+			string data = heldBack + new string(buffer, 0, length);
+			heldBack = length == charsRead ? string.Empty : buffer[charsRead - 1].ToString();
+
+			if (data.Length > 0)
+			{
+				onChunk(data);
+			}
+		}
+
+		if (heldBack.Length > 0)
+		{
+			onChunk(heldBack);
 		}
 	}