Combine pool and pool identity between netfx and netcore

[Wraith2]

Merged pool and pool identity versions between netcore and netfx.

Concerns:

• The two pool implementations work differently around wait handles. Both versions have the same comment that refers to WaitAny which is a managed method but the netfx implementation does complicated stuff with a SafeHandle holding a reference to unmanaged memory. I suspect that the netfx version is later than the netcore version because of the shared comment and that means that the best implementation is non-portable. This will need internal investigation from the MS side to identify what problem was being worked around and whether it can be done in a managed way.
• Performance counters were present in the netfx version and are not in the netcore version. The required classes could be stubbed into netcore but is there a better way? should the counters be replaced with a newer tech?
• I chose to use the netcore pool identity implementation and added in stub and replacements in netfx as needed. The managed code approach in netfx was not needed and so the security around it was not needed because that will not be done inside the framework.

[cheenamalhotra]

I chose to use the netcore pool identity implementation and added in stub and replacements in netfx as needed. The managed code approach in netfx was not needed and so the security around it was not needed because that will not be done inside the framework.

There are few things to note about DbConnectionPoolIdentity implementation:

1. About current NetFx implementation I have some doubts to whether it even works as designed. e.g. if (!UnsafeNativeMethods.GetTokenInformation(token, 1, tokenStruct, bufferLength, ref lengthNeeded)) condition has different behaviors when it resolves to true, which doesn't seem right in first glance. But I'm not very sure if we should just remove it and replace the whole thing with NetCore source directly. I'll need to do some testing to validate behaviors between NetFx/NetCore before making that decision.

2. Noticeable behavior change might be observed in ASP.NET applications if we gather Current User from Managed API Environment.UserName, which will be now used in .NET Framework driver instead of the driver making Native DLL calls as of now. As per documentation:

   "If an ASP.NET application runs in a development environment, the UserName property returns the name of the current user. In a published ASP.NET application, this property returns the name of the application pool account (such as Default AppPool)."

   In order to accept that change, we need to validate current behavior to ensure it doesn't cause any impact.

3. There's also a concept of lastIdentity in NetFx which seems to be caching pool identities, and may prove beneficial to adapt.

[cheenamalhotra]

Moving this to next milestone as we need more time to test/validate changes.

[cheenamalhotra]

"If an ASP.NET application runs in a development environment, the UserName property returns the name of the current user. In a published ASP.NET application, this property returns the name of the application pool account (such as Default AppPool)."

I've validated this is true, so we cannot change this behavior and must retain old style of username for ASP.NET compatible support.

[Wraith2]

I didn't think I'd changed that behaviour but as you said it is needed for compatibility. Consider this one un-mergable until I've come back and reviewed, that might take a while but I don't think we're in a rush for this.

[Wraith2]

Noticeable behavior change might be observed in ASP.NET applications if we gather Current User from Managed API Environment.UserName, which will be now used in .NET Framework driver instead of the driver making Native DLL calls as of now. As per documentation:

On windows we'll use the native implementation which uses WindowsIdentity and it has to do that to be impersonation aware. The same logic means that running in an app pool it will gwt the app pool uaer which will be the correct apsnet user name. The only time the Environment Username and Domain will be used is on non-windows os' which don't have suport for per-thread impersonation.

There's also a concept of lastIdentity in NetFx which seems to be caching pool identities, and may prove beneficial to adapt.

There is in netcore as well but it's in a slightly lower place in the object tree because there is the split between managed and native implementations. These changes still cache the last id and avoid allocating the new pool identify if it would have the same properties as the last one used.

As far as I can see the behaviour should be identical.

[Wraith2]

@lcheunglci as a merge expert would you mind looking at this set of changes and seeing what you think?

[lcheunglci]

Overall looks good to me, hopefully once your merge the latest changes <SubType>Component</SubType> in the csproj goes away

[Wraith2]

Thanks. Rebased and pushed.

[lcheunglci]

Looks good to me

[Wraith2]

This has been open 9 months now. If there isn't enough confidence to merge I can close and allow it to be handled along with all the other merges taking place, just needs a decision.

[JRahnama]

This has been open 9 months now. If there isn't enough confidence to merge I can close and allow it to be handled along with all the other merges taking place, just needs a decision.

This should be reviewed next week as well. We know it has been open for a long time and really sorry about it. We are doing are best to address the remaining PRs regarding merging code bases before next release, but you may know by now that how busy sometimes we are get in this team. Bear with us and we'll get to this soon. 🙏🏽

A reminder that we are off on Monday.

[Wraith2]

10 Months! I think the chances of being able to celebrate a birthday are fairly good.

[cheenamalhotra]

I've validated this is true, so we cannot change this behavior and must retain old style of username for ASP.NET compatible support.

Is this addressed? #862 (comment)
We haven't seen a commit to address this that's why this is on hold.

To clarify my earlier comment and answer yours later, the change of behavior is reproducible and therefore I asked to fix that change.

[Wraith2]

Can you provide your validation please? The underlying windows api used behind Environment.UserName (and DomainName) uses the identity of the user even if it's the asp.net user and respects impersonation, there should be no change.

[Wraith2]

Nevermind this. I tried to rebase to current and it just ends up in a huge mess of conflicts to resolve. I no longer have the patience to struggle with this individual PR given that others are now working on the process of merging the codebase.