Fix Console.Write on iOS #56713

MaximLipnin · 2021-08-02T09:42:46Z

This is iOS-part of Egor's fix (with Ralf's suggestion) from #37078.

For this code snippet:

Console.Write ("A");
Console.Write ("B");
Console.WriteLine ("C");

Console.Write ("D\nd");
Console.Write ("E\ne");
Console.WriteLine ("F");

the output will be:

2021-08-02 12:19:25.865365+0300 iOS.Simulator.ConsoleWrite.Test[4681:132934] ABC
2021-08-02 12:19:25.868503+0300 iOS.Simulator.ConsoleWrite.Test[4681:132934] D
2021-08-02 12:19:25.869020+0300 iOS.Simulator.ConsoleWrite.Test[4681:132934] dE
2021-08-02 12:19:25.869138+0300 iOS.Simulator.ConsoleWrite.Test[4681:132934] eF

ghost · 2021-08-02T09:42:56Z

Tagging subscribers to this area: @dotnet/area-system-console
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #36440.

This is iOS-part of Egor's fix from #37078.

For this code snippet:

Console.Write ("A");
Console.Write ("B");
Console.WriteLine ("C");

Console.Write ("D\nd");
Console.Write ("E\ne");
Console.WriteLine ("F");

the output will be:

2021-08-02 12:19:25.865365+0300 iOS.Simulator.ConsoleWrite.Test[4681:132934] ABC
2021-08-02 12:19:25.868503+0300 iOS.Simulator.ConsoleWrite.Test[4681:132934] D
2021-08-02 12:19:25.869020+0300 iOS.Simulator.ConsoleWrite.Test[4681:132934] dE
2021-08-02 12:19:25.869138+0300 iOS.Simulator.ConsoleWrite.Test[4681:132934] eF

Author:	MaximLipnin
Assignees:	-
Labels:	`area-System.Console`
Milestone:	-

src/libraries/System.Console/src/System/IO/ConsoleStream.cs

src/libraries/System.Console/src/System/ConsolePal.iOS.cs

am11 · 2021-08-02T10:11:25Z

Can we enable

runtime/src/libraries/System.Console/tests/ConsoleEncoding.cs

Lines 25 to 26 in 79c4144

    
           [SkipOnPlatform(TestPlatforms.Browser | TestPlatforms.iOS | TestPlatforms.MacCatalyst | TestPlatforms.tvOS, "Not supported on Browser, iOS, MacCatalyst, or tvOS.")] 
        
           public void TestEncoding(string inputString)

or any other test for this on iOS? (from the skip reason message of this test, it's not clear which API is not supported)

src/libraries/System.Console/src/System/IO/ConsoleStream.cs

MaximLipnin · 2021-08-02T15:13:23Z

Can we enable

runtime/src/libraries/System.Console/tests/ConsoleEncoding.cs

Lines 25 to 26 in 79c4144

[SkipOnPlatform(TestPlatforms.Browser | TestPlatforms.iOS | TestPlatforms.MacCatalyst | TestPlatforms.tvOS, "Not supported on Browser, iOS, MacCatalyst, or tvOS.")]

public void TestEncoding(string inputString)

or any other test for this on iOS? (from the skip reason message of this test, it's not clear which API is not supported)

Added TestConsoleWrite test

src/libraries/System.Console/src/System/ConsolePal.iOS.cs

MaximLipnin · 2021-08-10T12:13:13Z

It's quite close to a working state, the only problem is that I cannot find the right moment to write the data remaining in the cache to the stream before closing the app.

src/libraries/System.Private.CoreLib/src/System/IO/StreamWriter.cs

src/libraries/System.Console/tests/ReadAndWrite.cs

src/libraries/System.Private.CoreLib/src/System/IO/StreamWriter.cs

…pnin/runtime into fix_console_write_on_ios # Conflicts: # src/libraries/System.Console/src/System/ConsolePal.iOS.cs

src/libraries/System.Console/src/System/ConsolePal.iOS.cs

MaximLipnin · 2021-08-31T13:39:11Z

The PR looks pretty green now. @stephentoub Would you like to take another look?

src/libraries/System.Console/src/System/ConsolePal.iOS.cs

stephentoub · 2021-09-01T13:30:46Z

src/libraries/System.Console/src/System/ConsolePal.iOS.cs

+            }
+
+            // no newlines found, add the entire buffer to the cache
+            cache.Append(charBuffer);


General question about this change... it seems like this change means that if I have code that does:

while (true) { Console.Write("."); await Task.Delay(1000); }

to print out a period to the console once a second, for example as you might find with a progress indicator in a console app, that the data will never be emitted to the console, because newlines are never found. Am I understanding correctly? Is that not an issue, e.g. because Console is only used for diagnostic messages in an iOS app?

Yes, this is a thing I wasn't able to beat. I mentioned it a bit earlier #56713 (comment). The current Xamarin implementation has the same drawback so it won't surprise the existing customers.

src/libraries/System.Console/src/System/ConsolePal.iOS.cs

MaximLipnin · 2021-09-02T13:56:48Z

/backport to release/6.0

github-actions · 2021-09-02T13:57:02Z

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1194460075

github-actions · 2021-09-02T13:58:52Z

@MaximLipnin backporting to release/6.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Fix Console.Write on iOS
Applying: Move the helper method to iOS-specific location
Applying: Add a test
Applying: Remove unused using
Applying: Apply Ralf's suggestion
Applying: Move Interop.Sys.Log to a local function
Applying: Move the caching logic over to StreamWriter
Applying: feedback
Applying: Remove unused usings
Applying: Derive from StreamWriter
Applying: Feedback
Applying: Try a solution not changing public API surface and using NSLogStream with a decoder
Applying: Some feedback
Applying: Use ArrayPool
Applying: Not rely on the length of the rented array
Applying: Address the feedback
Applying: not use _ prefix for local variable
Applying: Address the feedback
Using index info to reconstruct a base tree...
M	src/libraries/System.Console/src/System/ConsolePal.iOS.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/System.Console/src/System/ConsolePal.iOS.cs
CONFLICT (content): Merge conflict in src/libraries/System.Console/src/System/ConsolePal.iOS.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0018 Address the feedback
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

A small cleanup after #56713.

Backport of #56713

Expands on dotnet#56713 by moving the caching implementation to a separate internal class leaving only the interop calls in ConsolePal.iOS and ConsolePal.Android Fixes dotnet#57304

Expands on #56713 by moving the caching implementation to a separate internal class leaving only the interop calls in ConsolePal.iOS and ConsolePal.Android Fixes #57304

Fix Console.Write on iOS

71b3888

MaximLipnin requested review from directhex, steveisok, akoeplinger and mdh1418 August 2, 2021 09:42

dotnet-issue-labeler bot added the area-System.Console label Aug 2, 2021

adamsitnik reviewed Aug 2, 2021

View reviewed changes

src/libraries/System.Console/src/System/IO/ConsoleStream.cs Outdated Show resolved Hide resolved

src/libraries/System.Console/src/System/ConsolePal.iOS.cs Outdated Show resolved Hide resolved

MaximLipnin added 2 commits August 2, 2021 16:36

Move the helper method to iOS-specific location

8f6f7bc

Add a test

a97f0d5

am11 reviewed Aug 2, 2021

View reviewed changes

src/libraries/System.Console/src/System/IO/ConsoleStream.cs Outdated Show resolved Hide resolved

MaximLipnin added 3 commits August 2, 2021 17:22

Remove unused using

90bf33c

Apply Ralf's suggestion

e6806e4

Move Interop.Sys.Log to a local function

2bdb6e7

MaximLipnin marked this pull request as ready for review August 2, 2021 15:11

stephentoub reviewed Aug 2, 2021

View reviewed changes

src/libraries/System.Console/src/System/ConsolePal.iOS.cs Outdated Show resolved Hide resolved

MaximLipnin added 2 commits August 3, 2021 10:50

Merge branch 'main' into fix_console_write_on_ios

612dc1c

Merge branch 'main' into fix_console_write_on_ios

6f8542b

adamsitnik added this to the 6.0.0 milestone Aug 9, 2021

MaximLipnin added 2 commits August 10, 2021 15:06

Move the caching logic over to StreamWriter

47d8f01

Merge branch 'main' into fix_console_write_on_ios

4f0653a

MaximLipnin commented Aug 10, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/StreamWriter.cs Outdated Show resolved Hide resolved

mdh1418 reviewed Aug 10, 2021

View reviewed changes

src/libraries/System.Console/tests/ReadAndWrite.cs Show resolved Hide resolved

am11 reviewed Aug 10, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/StreamWriter.cs Outdated Show resolved Hide resolved

am11 reviewed Aug 10, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/StreamWriter.cs Outdated Show resolved Hide resolved

MaximLipnin added 2 commits August 11, 2021 10:45

Merge branch 'main' into fix_console_write_on_ios

14545a8

feedback

725e64c

MaximLipnin added 3 commits August 27, 2021 19:39

Merge branch 'fix_console_write_on_ios' of https://github.com/maximli…

a234df5

…pnin/runtime into fix_console_write_on_ios # Conflicts: # src/libraries/System.Console/src/System/ConsolePal.iOS.cs

Fix an error

34d215f

Merge branch 'main' into fix_console_write_on_ios

1eb7528

stephentoub reviewed Aug 30, 2021

View reviewed changes

src/libraries/System.Console/src/System/ConsolePal.iOS.cs Outdated Show resolved Hide resolved

stephentoub reviewed Aug 30, 2021

View reviewed changes

src/libraries/System.Console/src/System/ConsolePal.iOS.cs Outdated Show resolved Hide resolved

Print a span rather than a string

a2e118e

stephentoub reviewed Aug 31, 2021

View reviewed changes

src/libraries/System.Console/src/System/ConsolePal.iOS.cs Outdated Show resolved Hide resolved

Slice the char span to the exact used length

732a62f

stephentoub reviewed Sep 1, 2021

View reviewed changes

src/libraries/System.Console/src/System/ConsolePal.iOS.cs Outdated Show resolved Hide resolved

stephentoub reviewed Sep 1, 2021

View reviewed changes

src/libraries/System.Console/src/System/ConsolePal.iOS.cs Outdated Show resolved Hide resolved

stephentoub approved these changes Sep 1, 2021

View reviewed changes

MaximLipnin added 2 commits September 1, 2021 17:12

Address Stephen's feedback

9375848

Remove a redundant using

7504d30

MaximLipnin merged commit faafb32 into dotnet:main Sep 2, 2021

MaximLipnin deleted the fix_console_write_on_ios branch September 2, 2021 13:53

This was referenced Sep 2, 2021

Fix Console.Write on iOS (#56713) #58559

Merged

Remove unnecessary unsafe keyword #58606

Merged

akoeplinger pushed a commit that referenced this pull request Sep 3, 2021

Remove unnecessary unsafe keyword (#58606)

d57196e

A small cleanup after #56713.

akoeplinger pushed a commit that referenced this pull request Sep 3, 2021

Fix Console.Write on iOS (#56713) (#58559)

41dcbd6

Backport of #56713

steveisok mentioned this pull request Sep 10, 2021

Use caching ConsoleStream for both iOS and Android #58967

Merged

ghost locked as resolved and limited conversation to collaborators Oct 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Console.Write on iOS #56713

Fix Console.Write on iOS #56713

MaximLipnin commented Aug 2, 2021 •

edited

Loading

ghost commented Aug 2, 2021

am11 commented Aug 2, 2021

MaximLipnin commented Aug 2, 2021

MaximLipnin commented Aug 10, 2021

MaximLipnin commented Aug 31, 2021

stephentoub Sep 1, 2021

MaximLipnin Sep 1, 2021

MaximLipnin commented Sep 2, 2021

github-actions bot commented Sep 2, 2021

github-actions bot commented Sep 2, 2021

Fix Console.Write on iOS #56713

Fix Console.Write on iOS #56713

Conversation

MaximLipnin commented Aug 2, 2021 • edited Loading

ghost commented Aug 2, 2021

am11 commented Aug 2, 2021

MaximLipnin commented Aug 2, 2021

MaximLipnin commented Aug 10, 2021

MaximLipnin commented Aug 31, 2021

stephentoub Sep 1, 2021

Choose a reason for hiding this comment

MaximLipnin Sep 1, 2021

Choose a reason for hiding this comment

MaximLipnin commented Sep 2, 2021

github-actions bot commented Sep 2, 2021

github-actions bot commented Sep 2, 2021

MaximLipnin commented Aug 2, 2021 •

edited

Loading