Equals/GetHashCode for UnixDomainSocketEndPoint #69722

sakno · 2022-05-24T15:40:29Z

ghost · 2022-05-24T15:40:36Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Author:	sakno
Assignees:	-
Labels:	`area-System.Net`
Milestone:	-

src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs

rzikm · 2022-05-25T10:59:31Z

I am not entirely sold on having Equals mutate internal state, but in this case seems harmless. Otherwise looks good, but I'd rather somebody else take a look as well, perhaps @antonfirsov?

sakno · 2022-05-25T11:32:19Z

@rzikm agree, but the allocation is effectively reused. So, during the app lifecycle, the amount of allocated memory remains the same.

wfurt · 2022-05-25T13:28:30Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs

@@ -10,13 +11,24 @@ namespace System.Net.Sockets
    /// <summary>Represents a Unix Domain Socket endpoint as a path.</summary>
    public sealed partial class UnixDomainSocketEndPoint : EndPoint
    {
+        // taken from System.IO.PathInternal


While this may be copy this is only best guess. Aside from Windows, the case (in)sensitivity is property of the file system e.g. while apfs is case insensitive by default, you can have case sensitive version as well. Similarly on Linux, ext2fs is case sensitive but you can have different filesystems (like vfat) that are not.

wfurt · 2022-05-25T13:34:56Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs

+        // Otherwise, two endpoints may not be equal even if they pointing to the same file.
+        // Lazily initialized field then can be reused by these methods to avoid further
+        // allocations.
+        private string? _fullPath;


It is somewhat surprising that GetHash or equality would need new state.

Not actually new, the value of this field is used for two purposes. One of them is Equals/GetHashCode, yes. Otherwise, two paths pointing to the same location in fs will give different hash codes for EndPoint.

However, we can assume that relative paths pointing to the same location give us different EndPoints. If so, I can change the behavior.

tmds · 2022-05-26T06:11:06Z

I am not entirely sold on having Equals mutate internal state, but in this case seems harmless. Otherwise looks good, but I'd rather somebody else take a look as well, perhaps @antonfirsov?

Imo Equals/GetHashCode should rely solely on the path that gets provided by the user and stored in the readonly field.
When we create the BoundEndPoint, that path remains the same.

The BoundFileName is a mutable field for internal usage. We need to call Path.GetFullPath to set its value only when the Socket.Bind is performed otherwise we may capture the working directory at a different time.

stephentoub · 2022-05-31T17:47:28Z

I'm worried about all the semantics being baked into equality here, attempting to say that the type's equality is based on the underlying OS behavior. Can we just give the instance basic value equality, e.g. public bool Equals(object obj) => obj is UnixDomainSocketEndPoint ep && _path == ep._path;? That is simple, explainable, cheap, and doesn't try to predict whatever semantics the OS might or might not provide for deciding the path maps to the same object as another (e.g. what if two paths actually pointed to the same location thanks to symlinks). This is just like DnsEndPoint, for example: it just compares the provided inputs for value equality, and doesn't e.g. try to resolve the hosts to see whether they'll end up using the same underlying IP address after DNS resolution.

sakno · 2022-05-31T19:27:21Z

Can we just give the instance basic value equality, e.g. public bool Equals(object obj) => obj is UnixDomainSocketEndPoint ep && _path == ep._path;?

I see no problem with such an approach. However, we can be still more accurate and try to normalize relative paths. However, without #2162 I can't do that.

Another thing - path comparison must take into account case sensitivity, IMO.

P.S.: Interesting observation, but DnsEndPoint is case-sensitive while RFC 4343 states that DNS labels are not case-sensitive. As a result, false is returned for the following code: new DnsEndPoint("localhost", 80).Equals(new DnsEndPoint("LOCALHOST", 80))

stephentoub · 2022-05-31T19:38:09Z

Another thing - path comparison must take into account case sensitivity, IMO.

I'm suggesting it not. I'm suggesting Equals/GetHashCode here be agnostic to OS path equivalence, which is entirely up to the operating system and can be significantly influenced by case-sensitivity per folder, by symlinks, etc. I'm suggesting Equals/GetHashCode be based entirely on the state passed in, i.e. a UnixDomainSocketEndPoint is equivalent to another if and only if its string path is Equal (ordinal case-sensitive), and nothing more. If someone wants more complicated semantics, they can use a custom comparer. If such semantics make Equals/GetHashCode overrides undesirable, then we shouldn't override them.

src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs

rzikm · 2022-06-07T16:17:53Z

There seem to be test failures (System.Net.Sockets.Tests.UnixDomainSocketTest.FilePathEquality) on Windows
@sakno Can you take a look at it?

If the functionality is not supported on Windows, you can simply annotate the tests with PlatformSpecificAttribute or SkipOnPlatformAttribute, see https://github.com/dotnet/runtime/blob/main/docs/workflow/testing/libraries/filtering-tests.md

sakno · 2022-06-07T16:22:04Z

@rzikm , sure. I'm using Linux, didn't see that error.

stephentoub · 2022-06-07T20:01:18Z

src/libraries/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs

+            Assert.NotEqual(endPoint1, endPoint3);
+            Assert.NotEqual(endPoint2, endPoint3);
+            Assert.NotEqual(endPoint1.GetHashCode(), endPoint3.GetHashCode());
+            Assert.NotEqual(endPoint2.GetHashCode(), endPoint3.GetHashCode());


These hashcode NotEqual asserts could sporadically fail. It should be exceedingly rare, but better to remove them. We could instead assert that the endpoint hashcode equals the path hashcode, and leave the correctness of the path hashcode up to string's implementation.

Same for the test below.

Because of hash code collision.. hmm, makes sense. I would prefer just to remove NotEqual assertions.

stephentoub

Thanks.

rzikm · 2022-06-08T07:58:42Z

Thanks for having the patience with this.

Added impl of Equals/GetHashCode for UnixDomainSocketEP

afc0431

dotnet-issue-labeler bot added the area-System.Net label May 24, 2022

ghost added the community-contribution Indicates that the PR has been added by a community member label May 24, 2022

wfurt requested a review from a team May 25, 2022 09:11

rzikm reviewed May 25, 2022

View reviewed changes

src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs Outdated Show resolved Hide resolved

Fixed typo

b308d73

rzikm reviewed May 25, 2022

View reviewed changes

src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs Outdated Show resolved Hide resolved

sakno added 2 commits May 25, 2022 15:52

Fixed missing underscore

90ddb6f

Clarify new fields using comments

1d1b9c2

wfurt reviewed May 25, 2022

View reviewed changes

karelz requested a review from stephentoub May 31, 2022 16:19

Review feedback

0fde07a

stephentoub reviewed May 31, 2022

View reviewed changes

src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs Outdated Show resolved Hide resolved

Review feedback

7e3d6ee

Added platform verification

843405e

rzikm requested a review from stephentoub June 7, 2022 19:46

stephentoub reviewed Jun 7, 2022

View reviewed changes

stephentoub approved these changes Jun 7, 2022

View reviewed changes

Fixed sporadic test failure due to hashcode collision

6b25d4f

rzikm merged commit fc8ce9b into dotnet:main Jun 8, 2022

maraf mentioned this pull request Jun 9, 2022

Invalid path length for use with domain sockets on iOS/tvOS platform #70468

Closed

ghost locked as resolved and limited conversation to collaborators Jul 8, 2022

karelz added this to the 7.0.0 milestone Jul 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Equals/GetHashCode for UnixDomainSocketEndPoint #69722

Equals/GetHashCode for UnixDomainSocketEndPoint #69722

sakno commented May 24, 2022

ghost commented May 24, 2022

rzikm commented May 25, 2022

sakno commented May 25, 2022

wfurt May 25, 2022

wfurt May 25, 2022

sakno May 25, 2022

tmds commented May 26, 2022

stephentoub commented May 31, 2022 •

edited

Loading

sakno commented May 31, 2022

stephentoub commented May 31, 2022 •

edited

Loading

rzikm commented Jun 7, 2022 •

edited

Loading

sakno commented Jun 7, 2022

stephentoub Jun 7, 2022

sakno Jun 7, 2022

stephentoub left a comment

rzikm commented Jun 8, 2022

Equals/GetHashCode for UnixDomainSocketEndPoint #69722

Equals/GetHashCode for UnixDomainSocketEndPoint #69722

Conversation

sakno commented May 24, 2022

ghost commented May 24, 2022

rzikm commented May 25, 2022

sakno commented May 25, 2022

wfurt May 25, 2022

Choose a reason for hiding this comment

wfurt May 25, 2022

Choose a reason for hiding this comment

sakno May 25, 2022

Choose a reason for hiding this comment

tmds commented May 26, 2022

stephentoub commented May 31, 2022 • edited Loading

sakno commented May 31, 2022

stephentoub commented May 31, 2022 • edited Loading

rzikm commented Jun 7, 2022 • edited Loading

sakno commented Jun 7, 2022

stephentoub Jun 7, 2022

Choose a reason for hiding this comment

sakno Jun 7, 2022

Choose a reason for hiding this comment

stephentoub left a comment

Choose a reason for hiding this comment

rzikm commented Jun 8, 2022

stephentoub commented May 31, 2022 •

edited

Loading

stephentoub commented May 31, 2022 •

edited

Loading

rzikm commented Jun 7, 2022 •

edited

Loading