Fix empty string received by .NET 8 users on Linux on userName

[gdziadkiewicz]

Background

While working on getting log4net.Ext.Json package to work with log4net 3 I noticed a regression. With log4net 2.0.10 the test checking if the user name is as expected worked both during local run on my Windows machine and on the CI Linux agent used by Gitlab. After upgrading log4net to version 3.0.1 it stopped working on the CI and started reporting that the user name is an empty string.

Passing:
https://gitlab.com/gdziadkiewicz/log4net.Ext.Json/-/jobs/7522362244

Failing:
https://gitlab.com/gdziadkiewicz/log4net.Ext.Json/-/jobs/8094529032#L172

Testing

It would be great to be able to run tests for log4net on all combinations of frameworks and OSes automatically (would you be interested in someone getting started work on it?).

In the meantime it draws as important for me to test (not sure about expected results yet):

• windows + net8
• windows + net6
• windows + net461
• linux + net8
• linux + net6
• mac + net8
• mac + net6
• linux + mono
• mac + mono
• windows + mono

Questions to answer before finalizing the change

1. Is WindowsIdentity Windows specific (name seems to suggest it)?
2. How is Environment.User implemented on different platforms? Does it change? Is it expensive? Would caching make sense?
3. Does and WindowsIdentity work on Mono on Win? If yes, how?
4. RuntimeInformation.IsOSPlatform is supported from net471, would reformulating the #if around it to communicate that help in the future?

[FreeAndNil]

added some hints

[gdziadkiewicz]

@FreeAndNil Thanks for the extra quick check of my draft! I will iterate on the suggestions and questions/stuff in description. What is the deadline for 3.0.3?

[FreeAndNil]

There is no deadline yet.
I think we will release in the middle of November, but this is negotiable 😅

[FreeAndNil]

Adding some unit tests would be very nice.

[gdziadkiewicz]

I will add the unit tests.

[FreeAndNil]

Hi @gdziadkiewicz,

trying to answer some of your questions.

Testing

It would be great to be able to run tests for log4net on all combinations of frameworks and OSes automatically (would you be interested in someone getting started work on it?).

Yes this would be really helpful. There is #106 and a github-actions branch from @rm5248 - I didn't find the time yet to take a closer look.

In the meantime it draws as important for me to test (not sure about expected results yet):

* windows + net8
* windows + net6
* windows + net461
* linux + net8
* linux + net6
* mac + net8
* mac + net6
* linux + mono
* mac + mono
* windows + mono

I think we can skip net6 (support ends in 3 weeks) and mono on windows (I don't think many users will do this).

Questions to answer before finalizing the change

1. Is WindowsIdentity Windows specific (name seems to suggest it)?

Yes. https://github.com/dotnet/runtime/tree/main/src/libraries/System.Security.Principal.Windows/src

2. How is `Environment.User` implemented on different platforms? Does it change? Is it expensive? Would caching make sense?

It should work on all platforms, but it seems to give different results in web applications.
See https://stackoverflow.com/questions/33665929/whats-the-difference-between-system-environment-username-and-user-identity-name

3. Does and WindowsIdentity work on Mono on Win? If yes, how?

Yes. Same as net471. https://github.com/mono/mono/blob/main/mcs/class/corlib/System.Security.Principal/WindowsIdentity.cs

4. [RuntimeInformation.IsOSPlatform](https://learn.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.runtimeinformation.isosplatform?view=netframework-4.7.1) is supported from net471, would reformulating the #if around it to communicate that help in the future?

Do you mean switching from net462 to net471 or adding a comment in the code?

Caching of the username seems not to be necessary. I get around 500 ms for 10.000 calls.

[gdziadkiewicz]

My plan was to find the answers alone, without using your time :D Thank you for the research!

Do you mean switching from net462 to net471 or adding a comment in the code?

Current code guarding the code is #if !NET462_OR_GREATER which, combined with us targeting net462 and netstandard20, means we won't include the call in any classic .net framework, only netstandard20. That works , but the way matching the docs would to only exclude it in net462 (cause that's where it is missing). Right now it doesn't matter, but if we retarget to net472 or something in the future that call which could be used there to check the OS easily will be excluded. So changing the #if would be future-proofing.

[gdziadkiewicz]

I added the unit test and tested on Windows.

[gdziadkiewicz]

@FreeAndNil I'm wondering about two parts of the code. One is the:

    catch
    {
      return null;
    }

Should it really be the only place that doesn't fail back to Environment.User

and the second is the first if utilizing the flag:

    if (_platformDoesNotSupportWindowsIdentity)
    {
      // we've already received one PlatformNotSupportedException or null from TryReadWindowsIdentityUserName
      // and it's highly unlikely that will change
      return Environment.UserName;
    }

It is the only piece of code outside of the try block. It would be highly unfortunate to get an unhandled exception from it(based on impl details Environment.UserName can throw ). WDYT?

[FreeAndNil]

My plan was to find the answers alone, without using your time :D Thank you for the research!

Do you mean switching from net462 to net471 or adding a comment in the code?

Current code guarding the code is #if !NET462_OR_GREATER which, combined with us targeting net462 and netstandard20, means we won't include the call in any classic .net framework, only netstandard20. That works , but the way matching the docs would to only exclude it in net462 (cause that's where it is missing). Right now it doesn't matter, but if we retarget to net472 or something in the future that call which could be used there to check the OS easily will be excluded. So changing the #if would be future-proofing.

Good point, you can adjust this.

[FreeAndNil]

@FreeAndNil I'm wondering about two parts of the code. One is the:

    catch
    {
      return null;
    }

Should it really be the only place that doesn't fail back to Environment.User

@fluffynuts you did the last meaningful change at this line in e137855 WDYT?

and the second is the first if utilizing the flag:

    if (_platformDoesNotSupportWindowsIdentity)
    {
      // we've already received one PlatformNotSupportedException or null from TryReadWindowsIdentityUserName
      // and it's highly unlikely that will change
      return Environment.UserName;
    }

It is the only piece of code outside of the try block. It would be highly unfortunate to get an unhandled exception from it(based on impl details Environment.UserName can throw ). WDYT?

We could catch errors and return NOTAVAILABLE.

BTW I recently activated the .net analyzers in #201 and changed all the catch-alls, so please merge master.

[gdziadkiewicz]

Hi, I implemented the suggestions, extended the try, and refreshed the branch with Master. It's good to be reviewed now :)

[FreeAndNil]

LGTM, thanks a lot.