From afc0431bde0e28a147866e494037d814a7472f84 Mon Sep 17 00:00:00 2001 From: sakno Date: Tue, 24 May 2022 18:31:36 +0300 Subject: [PATCH 1/8] Added impl of Equals/GetHashCode for UnixDomainSocketEP --- .../Net/Sockets/UnixDomainSocketEndPoint.cs | 61 +++++++++++++++++-- .../FunctionalTests/UnixDomainSocketTest.cs | 37 +++++++++++ 2 files changed, 94 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs index de3558da694fdc..a626162c079475 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Text; using System.IO; @@ -10,13 +11,24 @@ namespace System.Net.Sockets /// Represents a Unix Domain Socket endpoint as a path. public sealed partial class UnixDomainSocketEndPoint : EndPoint { + // taken from System.IO.PathInternal + private static readonly StringComparison s_filePathComparison = OperatingSystem.IsWindows() || + OperatingSystem.IsMacOS() || + OperatingSystem.IsIOS() || + OperatingSystem.IsTvOS() || + OperatingSystem.IsWatchOS() + ? StringComparison.OrdinalIgnoreCase + : StringComparison.Ordinal; + private const AddressFamily EndPointAddressFamily = AddressFamily.Unix; private readonly string _path; private readonly byte[] _encodedPath; + private readonly bool _bounded; + private string? _fullPath; // Tracks the file Socket should delete on Dispose. - internal string? BoundFileName { get; } + internal string? BoundFileName => _bounded ? _fullPath : null; public UnixDomainSocketEndPoint(string path) : this(path, null) @@ -26,7 +38,8 @@ private UnixDomainSocketEndPoint(string path, string? boundFileName) { ArgumentNullException.ThrowIfNull(path); - BoundFileName = boundFileName; + _bounded = boundFileName is not null; + _fullPath = boundFileName; // Pathname socket addresses should be null-terminated. // Linux abstract socket addresses start with a zero byte, they must not be null-terminated. @@ -124,21 +137,61 @@ public override string ToString() } } + [MemberNotNull(nameof(_fullPath))] + private void EnsurePathNormalized() + { + Debug.Assert(!IsAbstract(_path)); + + _fullPath ??= Path.GetFullPath(_path); + } + + public override bool Equals([NotNullWhen(true)] object? obj) + { + if (obj is not UnixDomainSocketEndPoint uds) + return false; + + switch ((IsAbstract(_path), IsAbstract(uds._path))) + { + case (true, true): + // abstract paths are case-sensitive on Windows and Unix-based systems + return MemoryExtensions.Equals(_path.AsSpan(1), uds._path.AsSpan(1), StringComparison.Ordinal); + case (false, false): + EnsurePathNormalized(); + uds.EnsurePathNormalized(); + return MemoryExtensions.Equals(_fullPath, uds._fullPath, s_filePathComparison); + default: + return false; + } + } + + public override int GetHashCode() + { + // abstract paths are case-sensitive on Windows and Unix-based systems + if (IsAbstract(_path)) + return string.GetHashCode(_path.AsSpan(1), StringComparison.Ordinal); + + EnsurePathNormalized(); + return string.GetHashCode(_fullPath, s_filePathComparison); + } + internal UnixDomainSocketEndPoint CreateBoundEndPoint() { if (IsAbstract(_path)) { return this; } - return new UnixDomainSocketEndPoint(_path, Path.GetFullPath(_path)); + + EnsurePathNormalized(); + return new UnixDomainSocketEndPoint(_path, _fullPath); } internal UnixDomainSocketEndPoint CreateUnboundEndPoint() { - if (IsAbstract(_path) || BoundFileName is null) + if (IsAbstract(_path) || !_bounded) { return this; } + return new UnixDomainSocketEndPoint(_path, null); } diff --git a/src/libraries/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs b/src/libraries/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs index 2e6b5037eea08b..28dc4276ae4d19 100644 --- a/src/libraries/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs +++ b/src/libraries/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs @@ -518,6 +518,43 @@ public void UnixDomainSocketEndPoint_RelativePathDeletesFile() }).Dispose(); } + [Fact] + public void AbstractPathEquality() + { + string abstractPath = '\0' + Guid.NewGuid().ToString(); + UnixDomainSocketEndPoint endPoint1 = new(abstractPath); + UnixDomainSocketEndPoint endPoint2 = new(abstractPath); + UnixDomainSocketEndPoint endPoint3 = new('\0' + Guid.NewGuid().ToString()); + + Assert.Equal(endPoint1, endPoint2); + Assert.Equal(endPoint1.GetHashCode(), endPoint2.GetHashCode()); + + Assert.NotEqual(endPoint1, endPoint3); + Assert.NotEqual(endPoint2, endPoint3); + Assert.NotEqual(endPoint1.GetHashCode(), endPoint3.GetHashCode()); + Assert.NotEqual(endPoint2.GetHashCode(), endPoint3.GetHashCode()); + } + + [Fact] + public void FilePathEquality() + { + string path1 = "filename"; + string path2 = "." + Path.DirectorySeparatorChar + "filename"; + string path3 = GetRandomNonExistingFilePath(); + + UnixDomainSocketEndPoint endPoint1 = new(path1); + UnixDomainSocketEndPoint endPoint2 = new(path2); + UnixDomainSocketEndPoint endPoint3 = new(path3); + + Assert.Equal(endPoint1, endPoint2); + Assert.Equal(endPoint1.GetHashCode(), endPoint2.GetHashCode()); + + Assert.NotEqual(endPoint1, endPoint3); + Assert.NotEqual(endPoint2, endPoint3); + Assert.NotEqual(endPoint1.GetHashCode(), endPoint3.GetHashCode()); + Assert.NotEqual(endPoint2.GetHashCode(), endPoint3.GetHashCode()); + } + private static string GetRandomNonExistingFilePath() { string result; From b308d731b3902e030fbcd68f5d55c550bcf3e4d9 Mon Sep 17 00:00:00 2001 From: sakno Date: Wed, 25 May 2022 13:25:34 +0300 Subject: [PATCH 2/8] Fixed typo --- .../src/System/Net/Sockets/UnixDomainSocketEndPoint.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs index a626162c079475..fb160ddcbdbd97 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs @@ -24,11 +24,11 @@ public sealed partial class UnixDomainSocketEndPoint : EndPoint private readonly string _path; private readonly byte[] _encodedPath; - private readonly bool _bounded; + private readonly bool isBound; private string? _fullPath; // Tracks the file Socket should delete on Dispose. - internal string? BoundFileName => _bounded ? _fullPath : null; + internal string? BoundFileName => isBound ? _fullPath : null; public UnixDomainSocketEndPoint(string path) : this(path, null) @@ -38,7 +38,7 @@ private UnixDomainSocketEndPoint(string path, string? boundFileName) { ArgumentNullException.ThrowIfNull(path); - _bounded = boundFileName is not null; + isBound = boundFileName is not null; _fullPath = boundFileName; // Pathname socket addresses should be null-terminated. @@ -187,7 +187,7 @@ internal UnixDomainSocketEndPoint CreateBoundEndPoint() internal UnixDomainSocketEndPoint CreateUnboundEndPoint() { - if (IsAbstract(_path) || !_bounded) + if (IsAbstract(_path) || !isBound) { return this; } From 90ddb6f3951cdc47232fdfa60ca9a4dfd650268e Mon Sep 17 00:00:00 2001 From: sakno Date: Wed, 25 May 2022 15:52:49 +0300 Subject: [PATCH 3/8] Fixed missing underscore --- .../src/System/Net/Sockets/UnixDomainSocketEndPoint.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs index fb160ddcbdbd97..77f4842fbf1fda 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs @@ -24,11 +24,11 @@ public sealed partial class UnixDomainSocketEndPoint : EndPoint private readonly string _path; private readonly byte[] _encodedPath; - private readonly bool isBound; + private readonly bool _isBound; private string? _fullPath; // Tracks the file Socket should delete on Dispose. - internal string? BoundFileName => isBound ? _fullPath : null; + internal string? BoundFileName => _isBound ? _fullPath : null; public UnixDomainSocketEndPoint(string path) : this(path, null) @@ -38,7 +38,7 @@ private UnixDomainSocketEndPoint(string path, string? boundFileName) { ArgumentNullException.ThrowIfNull(path); - isBound = boundFileName is not null; + _isBound = boundFileName is not null; _fullPath = boundFileName; // Pathname socket addresses should be null-terminated. @@ -187,7 +187,7 @@ internal UnixDomainSocketEndPoint CreateBoundEndPoint() internal UnixDomainSocketEndPoint CreateUnboundEndPoint() { - if (IsAbstract(_path) || !isBound) + if (IsAbstract(_path) || !_isBound) { return this; } From 1d1b9c2caf1a18e927c2e5e2108e9f91707b3fda Mon Sep 17 00:00:00 2001 From: sakno Date: Wed, 25 May 2022 16:03:17 +0300 Subject: [PATCH 4/8] Clarify new fields using comments --- .../System/Net/Sockets/UnixDomainSocketEndPoint.cs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs index 77f4842fbf1fda..2243b6c446a9cc 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs @@ -24,7 +24,21 @@ public sealed partial class UnixDomainSocketEndPoint : EndPoint private readonly string _path; private readonly byte[] _encodedPath; + + // The field is needed to distinguish the situation when + // _fullPath is not null but the object doesn't bound + // to the full path. _fullPath can be initialized by Equals/GetHashCode + // but it doesn't mean that the EndPoint is bound to the file system object. private readonly bool _isBound; + + // The field can be initialized lazily in the following circumstances: + // 1. Inside of Equals method + // 2. Inside of GetHashCode method + // 3. Inside of CreateBoundEndPoint method + // In case of non-abstract path, we need to have full path to the file system object. + // 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; // Tracks the file Socket should delete on Dispose. From 0fde07a9e7b9ac9a2a7d6d7b376ed9f6f6173fa0 Mon Sep 17 00:00:00 2001 From: sakno Date: Tue, 31 May 2022 22:58:17 +0300 Subject: [PATCH 5/8] Review feedback --- .../Net/Sockets/UnixDomainSocketEndPoint.cs | 70 +++---------------- .../FunctionalTests/UnixDomainSocketTest.cs | 4 +- 2 files changed, 10 insertions(+), 64 deletions(-) diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs index 2243b6c446a9cc..882339cb86945b 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs @@ -11,38 +11,13 @@ namespace System.Net.Sockets /// Represents a Unix Domain Socket endpoint as a path. public sealed partial class UnixDomainSocketEndPoint : EndPoint { - // taken from System.IO.PathInternal - private static readonly StringComparison s_filePathComparison = OperatingSystem.IsWindows() || - OperatingSystem.IsMacOS() || - OperatingSystem.IsIOS() || - OperatingSystem.IsTvOS() || - OperatingSystem.IsWatchOS() - ? StringComparison.OrdinalIgnoreCase - : StringComparison.Ordinal; - private const AddressFamily EndPointAddressFamily = AddressFamily.Unix; private readonly string _path; private readonly byte[] _encodedPath; - // The field is needed to distinguish the situation when - // _fullPath is not null but the object doesn't bound - // to the full path. _fullPath can be initialized by Equals/GetHashCode - // but it doesn't mean that the EndPoint is bound to the file system object. - private readonly bool _isBound; - - // The field can be initialized lazily in the following circumstances: - // 1. Inside of Equals method - // 2. Inside of GetHashCode method - // 3. Inside of CreateBoundEndPoint method - // In case of non-abstract path, we need to have full path to the file system object. - // 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; - // Tracks the file Socket should delete on Dispose. - internal string? BoundFileName => _isBound ? _fullPath : null; + internal string? BoundFileName { get; } public UnixDomainSocketEndPoint(string path) : this(path, null) @@ -52,8 +27,7 @@ private UnixDomainSocketEndPoint(string path, string? boundFileName) { ArgumentNullException.ThrowIfNull(path); - _isBound = boundFileName is not null; - _fullPath = boundFileName; + BoundFileName = boundFileName; // Pathname socket addresses should be null-terminated. // Linux abstract socket addresses start with a zero byte, they must not be null-terminated. @@ -151,41 +125,14 @@ public override string ToString() } } - [MemberNotNull(nameof(_fullPath))] - private void EnsurePathNormalized() - { - Debug.Assert(!IsAbstract(_path)); - - _fullPath ??= Path.GetFullPath(_path); - } - public override bool Equals([NotNullWhen(true)] object? obj) - { - if (obj is not UnixDomainSocketEndPoint uds) - return false; - - switch ((IsAbstract(_path), IsAbstract(uds._path))) - { - case (true, true): - // abstract paths are case-sensitive on Windows and Unix-based systems - return MemoryExtensions.Equals(_path.AsSpan(1), uds._path.AsSpan(1), StringComparison.Ordinal); - case (false, false): - EnsurePathNormalized(); - uds.EnsurePathNormalized(); - return MemoryExtensions.Equals(_fullPath, uds._fullPath, s_filePathComparison); - default: - return false; - } - } + => obj is UnixDomainSocketEndPoint ep && MemoryExtensions.SequenceEqual(_encodedPath, ep._encodedPath); public override int GetHashCode() { - // abstract paths are case-sensitive on Windows and Unix-based systems - if (IsAbstract(_path)) - return string.GetHashCode(_path.AsSpan(1), StringComparison.Ordinal); - - EnsurePathNormalized(); - return string.GetHashCode(_fullPath, s_filePathComparison); + HashCode hash = default; + hash.AddBytes(_encodedPath); + return hash.ToHashCode(); } internal UnixDomainSocketEndPoint CreateBoundEndPoint() @@ -195,13 +142,12 @@ internal UnixDomainSocketEndPoint CreateBoundEndPoint() return this; } - EnsurePathNormalized(); - return new UnixDomainSocketEndPoint(_path, _fullPath); + return new UnixDomainSocketEndPoint(_path, Path.GetFullPath(_path)); } internal UnixDomainSocketEndPoint CreateUnboundEndPoint() { - if (IsAbstract(_path) || !_isBound) + if (IsAbstract(_path) || BoundFileName is null) { return this; } diff --git a/src/libraries/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs b/src/libraries/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs index 28dc4276ae4d19..59fd20e5ee13c6 100644 --- a/src/libraries/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs +++ b/src/libraries/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs @@ -538,8 +538,8 @@ public void AbstractPathEquality() [Fact] public void FilePathEquality() { - string path1 = "filename"; - string path2 = "." + Path.DirectorySeparatorChar + "filename"; + string path1 = "relative" + Path.DirectorySeparatorChar + "path"; + string path2 = new(path1); // make a copy to avoid reference equality string path3 = GetRandomNonExistingFilePath(); UnixDomainSocketEndPoint endPoint1 = new(path1); From 7e3d6eebacd7357b04680a3f69e07fc2d1351c06 Mon Sep 17 00:00:00 2001 From: sakno Date: Tue, 31 May 2022 23:19:04 +0300 Subject: [PATCH 6/8] Review feedback --- .../src/System/Net/Sockets/UnixDomainSocketEndPoint.cs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs index 882339cb86945b..3bf4c8de4d79de 100644 --- a/src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs +++ b/src/libraries/System.Net.Sockets/src/System/Net/Sockets/UnixDomainSocketEndPoint.cs @@ -126,14 +126,9 @@ public override string ToString() } public override bool Equals([NotNullWhen(true)] object? obj) - => obj is UnixDomainSocketEndPoint ep && MemoryExtensions.SequenceEqual(_encodedPath, ep._encodedPath); + => obj is UnixDomainSocketEndPoint ep && _path == ep._path; - public override int GetHashCode() - { - HashCode hash = default; - hash.AddBytes(_encodedPath); - return hash.ToHashCode(); - } + public override int GetHashCode() => _path.GetHashCode(); internal UnixDomainSocketEndPoint CreateBoundEndPoint() { From 843405e033307ac4ffaa91d7c12d25dc927eb5a1 Mon Sep 17 00:00:00 2001 From: sakno Date: Tue, 7 Jun 2022 20:01:26 +0300 Subject: [PATCH 7/8] Added platform verification --- .../tests/FunctionalTests/UnixDomainSocketTest.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs b/src/libraries/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs index 59fd20e5ee13c6..aa7d73b7d87828 100644 --- a/src/libraries/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs +++ b/src/libraries/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs @@ -518,7 +518,7 @@ public void UnixDomainSocketEndPoint_RelativePathDeletesFile() }).Dispose(); } - [Fact] + [ConditionalFact(nameof(PlatformSupportsUnixDomainSockets))] public void AbstractPathEquality() { string abstractPath = '\0' + Guid.NewGuid().ToString(); @@ -535,7 +535,7 @@ public void AbstractPathEquality() Assert.NotEqual(endPoint2.GetHashCode(), endPoint3.GetHashCode()); } - [Fact] + [ConditionalFact(nameof(PlatformSupportsUnixDomainSockets))] public void FilePathEquality() { string path1 = "relative" + Path.DirectorySeparatorChar + "path"; From 6b25d4fc74b1eb205255372298d98ca35a430f91 Mon Sep 17 00:00:00 2001 From: sakno Date: Tue, 7 Jun 2022 23:13:45 +0300 Subject: [PATCH 8/8] Fixed sporadic test failure due to hashcode collision --- .../tests/FunctionalTests/UnixDomainSocketTest.cs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/libraries/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs b/src/libraries/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs index aa7d73b7d87828..a5491f6e3c1c4b 100644 --- a/src/libraries/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs +++ b/src/libraries/System.Net.Sockets/tests/FunctionalTests/UnixDomainSocketTest.cs @@ -531,8 +531,6 @@ public void AbstractPathEquality() Assert.NotEqual(endPoint1, endPoint3); Assert.NotEqual(endPoint2, endPoint3); - Assert.NotEqual(endPoint1.GetHashCode(), endPoint3.GetHashCode()); - Assert.NotEqual(endPoint2.GetHashCode(), endPoint3.GetHashCode()); } [ConditionalFact(nameof(PlatformSupportsUnixDomainSockets))] @@ -551,8 +549,6 @@ public void FilePathEquality() Assert.NotEqual(endPoint1, endPoint3); Assert.NotEqual(endPoint2, endPoint3); - Assert.NotEqual(endPoint1.GetHashCode(), endPoint3.GetHashCode()); - Assert.NotEqual(endPoint2.GetHashCode(), endPoint3.GetHashCode()); } private static string GetRandomNonExistingFilePath()