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

Android + Console.WriteLine + format strings = possible unexpected extra newlines #57304

Closed
jonpryor opened this issue Aug 12, 2021 · 2 comments · Fixed by #58967
Closed

Android + Console.WriteLine + format strings = possible unexpected extra newlines #57304

jonpryor opened this issue Aug 12, 2021 · 2 comments · Fixed by #58967
Milestone

Comments

@jonpryor
Copy link
Member

Description

Context: dotnet/android#6119
Context: dotnet/android#6119 (comment)

.NET for Android "forwards" Console.WriteLine() invocations to adb logcat. This is good and proper.

However, messages are written to adb logcat via __android_log_print, and the __android_log* family of functions emits a newline after every invocation.

An odd "hiccup" is that if you use Console.WriteLine() with format strings to write a string which exceeds 11 characters (?!) in length, the value is written to adb logcat across two or more lines:

using System;
using Android.App;

namespace android_hw
{
    [Activity(Label = "@string/app_name", MainLauncher = true)]
    public class MainActivity : Activity
    {
        protected override void OnCreate(Android.OS.Bundle savedInstanceState)
        {
            base.OnCreate(savedInstanceState);
            SetContentView(Resource.Layout.activity_main);

            Console.WriteLine ("{0}", new string ('x', 12));
        }
    }
}

…build & run…

% adb logcat > log.txt &
% ~/android-toolchain/dotnet/dotnet build /t:Install,StartAndroidActivity

…and adb logcat contains:

08-12 13:44:14.019 21107 21107 I DOTNET  : xxxxxxxxxxx
08-12 13:44:14.019 21107 21107 I DOTNET  : x

…which is, you know, weird.

There also doesn't appear to be any rhyme or reason for where the line wrap occurs, e.g. dotnet/android#6119 (comment)

try {
    throw new System.Exception("CRASH");
}
catch(System.Exception e)
{
    Console.WriteLine("# Unhandled Exception: e.ExceptionObject={0}", e);
}

results in:

08-10 16:02:49.498 16240 16267 I DOTNET  : # Unhandled Exception: e.ExceptionObject=System.Exce
08-10 16:02:49.498 16240 16267 I DOTNET  : ption: CRASH
08-10 16:02:49.498 16240 16267 I DOTNET  :    at Program.Main(String[] args) in /Users/thaysgrazia/runtime/src/mono/sample/Android/Program.cs:line 63

Why does this matter?

Sometimes you write tooling which reads adb logcat to see if a message was printed. It is extremely handy if what you get is what you expect, and nobody expects "randomly inserted" newlines.

For example, xamarin-android unit tests.

For comparison, "traditional"/"legacy" Xamarin.Android only splits on \n boundaries before sending to __android_log_print(), so no "unexpected" newlines are present in adb logcat with legacy Xamarin.Android.

Conjecture:

What might be happening is StreamWriter.WriteSpan():

while (count > 0)
{
if (dstPos == charBuffer.Length)
{
Flush(false, false);
dstPos = 0;
}
int n = Math.Min(charBuffer.Length - dstPos, count);
int bytesToCopy = n * sizeof(char);
Buffer.MemoryCopy(srcPtr, dstPtr + dstPos, bytesToCopy, bytesToCopy);
_charPos += n;
dstPos += n;
srcPtr += n;
count -= n;
}

StreamWriter.Flush(bool, bool) calls LogcatStream.Write(ReadOnlySpan<byte>):

public override unsafe void Write(ReadOnlySpan<byte> buffer)

Interop.Logcat.AndroidLogPrint(), meanwhile, is __android_log_print():

__android_log_print(level, tag, "%s", message, IntPtr.Zero);

Every time __android_log_print() is invoked, a newline is written to adb logcat.

Thus, if the while loop in StreamWriter.WriteSpan() is executed more than once while trying to write the # Unhandled Exception: message, that could explain why CRASH is written on a separate line, and would explain what @thaystg described elsewhere.

Configuration

  • Which version of .NET is the code running on? dotnet/installer@db9e71c7 6.0.100-rc.1.21408
  • What OS and version, and what distro if applicable? Android 11 on a Pixel 5, plus various other Android targets
  • What is the architecture (x64, x86, ARM, ARM64)? arm64, x86_64 (via emulator)
  • Do you know whether it is specific to that configuration? It doesn't appear to be specific to Android version or hardware. It instead appears to be specific to the Android-related .NET PAL & related code.

Regression?

This is not a .NET regression, but it is a "regression" or "semantic change" from legacy Xamarin.Android.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Aug 12, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

jonpryor pushed a commit to dotnet/android that referenced this issue Aug 12, 2021
Context: c1a2ee7
Context: dotnet/runtime#44526
Context: dotnet/runtime#44526 (comment)
Context: dotnet/runtime#44526 (comment)
Context: start: https://discord.com/channels/732297728826277939/732297837953679412/869330822262587392
Context:   end? https://discord.com/channels/732297728826277939/732297837953679412/869343082552893440
Context: dotnet/runtime#57304

Now that we are calling `mono_unhandled_exception()` when an unhandled
exception is reported (c1a2ee7), we should be able re-enable the
`InstallAndRunTests.SubscribeToAppDomainUnhandledException()` unit
test on .NET 6, which was disabled in 6e3e383.

What seemed like it should have been a 1-line removal ballooned a bit
due to a confluence of factors:

 1. Debugger component stubs, and
 2. .NET 6 `Console.WriteLine()` behavior.

On .NET 6, `JNIEnv.mono_unhandled_exception()` is
`monodroid_debugger_unhandled_exception()`, which calls
`mono_debugger_agent_unhandled_exception()`; see also e4debf7.

The problem is that in our current world order of "Mono components"
(0f7a0cd), if the debugger isn't used, then we get "debugger stubs"
for the mono debugger agent, which turns
`mono_debugger_agent_unhandled_exception()` into an [*assertion*][0]:

	static void
	stub_debugger_unhandled_exception (MonoException *exc)
	{
		g_assert_not_reached ();
	}

The result is that when an exception is thrown, *before* the
`AppDomain.UnhandledException` event can be raised, the runtime dies
in a horrible flaming assertion death:

	E andledexceptio: * Assertion: should not be reached at /__w/1/s/src/mono/mono/component/debugger-stub.c:175

Avoid this issue by checking `Debugger.IsAttached` *before* calling
`monodroid_debugger_unhandled_exception()`.

Additionally, remove some obsolete comments: .NET 6 couldn't resolve
`Debugger.Mono_UnhandledException()` because .NET 6 never had it, so
the linker was right to warn about its absence.

The problem with .NET 6 & `Console.WriteLine()` is that when format
strings are used, the output may be line-wrapped in unexpected ways;
see also dotnet/runtime@#57304.  Additionally, the `sender` parameter
value differs under .NET 6.  These two issues broke our unit test;
we *expected* `adb logcat` to contain:

	# Unhandled Exception: sender=RootDomain; e.IsTerminating=True; e.ExceptionObject=System.Exception: CRASH

Under .NET 6, `adb logcat` *instead* contained:

	# Unhandled Exception: sender=System.Object; e.IsTerminating=True; e.ExceptionObject=System.Exception
	: CRASH

Yes, `: CRASH` was on a separate `adb logcat` line.

Fix the `SubscribeToAppDomainUnhandledException()` unit test so that
`adb logcat` messages can now span multiple lines (which is sure to
cause all sorts of GC garbage!), and update the expected message so
that we look for the right message under legacy & .NET 6.

Co-authored-by: Jonathan Pryor <[email protected]>

[0]: https://github.com/dotnet/runtime/blob/16b456426dfb5212a24bfb78bfd5d9adfcc95185/src/mono/mono/component/debugger-stub.c#L172-L176
@steveisok steveisok removed the untriaged New issue has not been triaged by the area owner label Aug 18, 2021
@steveisok steveisok added this to the 7.0.0 milestone Aug 18, 2021
@steveisok
Copy link
Member

/cc @MaximLipnin

I'm marking the milestone as 7, but will consider fixing as we have something similar almost in place for iOS.

steveisok pushed a commit to steveisok/runtime that referenced this issue Sep 10, 2021
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
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Sep 10, 2021
steveisok added a commit that referenced this issue Sep 14, 2021
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
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Sep 14, 2021
github-actions bot pushed a commit that referenced this issue Sep 14, 2021
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
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants