Make Uri Thread-Safe

[MihaZupan]

Fixes #19130, contributes to #24239 (I will update the docs after we ensure thread-safety across all members).

Instead of locking on a string (could have even been the same instance the user passed into the ctor) and on the _info, we can perform the necesarry synchronization around Flags by using Interlocked.

UriInfo and MoreInfo fields are updated without synhronization, meaning that we may perform some duplicated work but not corrupt the state, The fields are always assigned-only in one go / written to before we share the instance (for example, we will set some UriInfo.Offset fields before saving the local instance to the shared _info field),

[MihaZupan]

Uri tests in CI are hitting this assert in threads.cpp. @jkotas can you comment on what that is about?

[jkotas]

Uri tests in CI are hitting this assert in threads.cpp. @jkotas can you comment on what that is about?

This assert means that lock was taken when thread exited. Your change might have exposed a bug somewhere else.

[stephentoub]

This assert means that lock was taken when thread exited. Your change might have exposed a bug somewhere else.

Presumably it's because of this line in the test?
https://github.com/dotnet/runtime/pull/33042/files#diff-8b7a4bf942ba035dbe2451db20151f71R1005

[jkotas]

Presumably it's because of this line in the test?

Yes, this change would trigger this assert. This assert is a left-over from SQL CLR days where it was important to avoid orphaned locks. I will look into deleting it.

[jkotas]

. This assert is a left-over from SQL CLR days where it was important to avoid orphaned locks. I will look into deleting it.

#33048

[MihaZupan]

The scope of this PR has changed to Uri thread safety as a whole, not just the lock-on-string behavior.

[MihaZupan]

@stephentoub could you take a look at these changes?

The gist of this change is

• Don't lock on the input string
• Swap locks for Interlocked operations
• Add some validation to prevent custom parsers from corrupting Uri instances
• Remove a (very hard to hit) race condition where Uri.Equals could return the wrong result
• Reduce race conditions around MoreInfo accesses, that would result in throwing away a value instead of caching it (not dangerous, just potentially duplicated work)

[stephentoub]

Looks pretty good. Have you measured perf impact of constructing and initializing Uris?

[MihaZupan]

Have you measured perf impact of constructing and initializing Uris?

I'm seeing a ~3% time improvement on the following test cases

[Benchmark]
public int NewUri_Port() => new Uri("https://microsoft.com").Port;

[Benchmark]
public Uri NewUri_WithUnicode() => new Uri("https://microsoft.com/ä");

[stephentoub]

Thanks.