[Uri] FxCop: System.Uri locks on a string.

[alexperovich]

This came from running FxCop analyzers on corefx. Reproduce steps found here.

"E:\corefx\src\src.builds" (default target) (1) ->
"E:\corefx\src\System.Private.Uri\src\System.Private.Uri.builds" (Build target) (293) ->
"E:\corefx\src\System.Private.Uri\src\System.Private.Uri.csproj" (Build target) (44:7) ->
System\Uri.cs(2427,19): error CA2002: Do not lock on a reference of type 'string' as it has weak identity. Replace that with a lock against an object with strong identity. [E:\corefx\src\System.Private.Uri\src\System.Private.Uri.csproj]

[karelz]

What is the context/source of this bug?

[alexperovich]

FxCop analyzer code analysis. I will add repro steps.

[svick]

I think this is a duplicate of https://github.com/dotnet/corefx/issues/11471.

[karelz]

From the dupe bug comment: https://github.com/dotnet/corefx/issues/11471#issue-175290581

https://github.com/dotnet/corefx/blob/ca9e2b068e75898a3b35860632b1fec01cc5af93/src/System.Private.Uri/src/System/Uri.cs#L2199-L2206

            lock (_string)
            {
                if ((_flags & Flags.MinimalUriInfoSet) == 0)
                {
                    _info = info;
                    _flags = (_flags & ~Flags.IndexMask) | cF;
                }
            }

This is a scalability concern, e.g. this Uri could end up sharing the same lock as another that's locking on the same string instance, such as if the string is interned. Now, given the rarity of this, this might be the best option, as unless there's another object available on which to lock, this is likely the better option than allocating a new object per Uri just for the purposes of this lock. If there's another existing object that could be used that's not shared, that would be better. Even better would be getting rid of this lock entirely... since Uri to my knowledge is not deemed to be thread-safe, the purpose of this lock is unclear to me... maybe it could just be removed entirely?

And comment https://github.com/dotnet/corefx/issues/11471#issuecomment-245026819

Uri is immutable and I think that people often assume (correctly or not) that reading the same object from multiple threads is fine, so it makes sense to me to make Uri thread-safe.

And it seems Uri was designed to be thread-safe, since there are some lock (_info) in it and also some comments about multi-threading.

Maybe it would make sense to verify that Uri indeed is thread-safe and then change the documentation?

[davidsh]

I would not change this lock. It might change behavior and System.Uri has large impacts on the codebase.

[davidsh]

cc: @CIPop

[danmoseley]

@CIPop if you agree, then @alexperovich can baseline and close this.

[CIPop]

This came from running FxCop analyzers on corefx.

@danmosemsft I agree with @alexperovich that the initial FXCop bug should be fixed (not baselined).
In general, I think we should have a clean analysis log (#3501).

Maybe it would make sense to verify that Uri indeed is thread-safe and then change the documentation?

@karelz This is a good idea but we would need to do some perf analysis first: would making it thread-safe loose a lot of perf?

[alexperovich]

I can baseline the error and file an issue to fix. Added to dotnet/corefx#14095

[CIPop]

We need to remove all instances where the Uri code is locking on a string and replace them with a proper lock objects.

Uri is immutable and I think that people often assume (correctly or not) that reading the same object from multiple threads is fine, so it makes sense to me to make Uri thread-safe.

I think this is a different, more complicated task and it shouldn't be addressed part of this issue.