From a4d84bf2787a3441a7adab66f3a266b533e08841 Mon Sep 17 00:00:00 2001 From: MarekM25 Date: Wed, 28 Jun 2023 15:19:46 +0200 Subject: [PATCH 1/7] Handling null in Transfer method --- src/DotNetty.Common/ThreadLocalPool.cs | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/DotNetty.Common/ThreadLocalPool.cs b/src/DotNetty.Common/ThreadLocalPool.cs index 327cb15e..c4819bc5 100644 --- a/src/DotNetty.Common/ThreadLocalPool.cs +++ b/src/DotNetty.Common/ThreadLocalPool.cs @@ -236,7 +236,7 @@ internal void Add(DefaultHandle handle) // transfer as many items as we can from this queue to the stack, returning true if any were transferred internal bool Transfer(Stack dst) { - Link head = this.head.link; + Link head = this.head?.link; if (head == null) { return false; @@ -259,6 +259,11 @@ internal bool Transfer(Stack dst) return false; } + if (dst?.elements == null) + { + return false; + } + int dstSize = dst.size; int expectedCapacity = dstSize + srcSize; @@ -273,9 +278,19 @@ internal bool Transfer(Stack dst) DefaultHandle[] srcElems = head.elements; DefaultHandle[] dstElems = dst.elements; int newDstSize = dstSize; + if (head.elements == null) + { + return false; + } + for (int i = srcStart; i < srcEnd; i++) { DefaultHandle element = srcElems[i]; + if (element == null) + { + return false; + } + if (element.recycleId == 0) { element.recycleId = element.lastRecycledId; From a1fcc1ba60046f74bb9ce465a7b7d1a74d74ec6c Mon Sep 17 00:00:00 2001 From: MarekM25 Date: Wed, 28 Jun 2023 15:45:19 +0200 Subject: [PATCH 2/7] try-catch for null reference in Transfer --- src/DotNetty.Common/ThreadLocalPool.cs | 137 +++++++++++++------------ 1 file changed, 74 insertions(+), 63 deletions(-) diff --git a/src/DotNetty.Common/ThreadLocalPool.cs b/src/DotNetty.Common/ThreadLocalPool.cs index c4819bc5..2ea2fc89 100644 --- a/src/DotNetty.Common/ThreadLocalPool.cs +++ b/src/DotNetty.Common/ThreadLocalPool.cs @@ -236,98 +236,109 @@ internal void Add(DefaultHandle handle) // transfer as many items as we can from this queue to the stack, returning true if any were transferred internal bool Transfer(Stack dst) { - Link head = this.head?.link; - if (head == null) + try { - return false; - } - - if (head.ReadIndex == LinkCapacity) - { - if (head.next == null) + Link head = this.head?.link; + if (head == null) { return false; } - this.head.link = head = head.next; - } - int srcStart = head.ReadIndex; - int srcEnd = head.WriteIndex; - int srcSize = srcEnd - srcStart; - if (srcSize == 0) - { - return false; - } + if (head.ReadIndex == LinkCapacity) + { + if (head.next == null) + { + return false; + } - if (dst?.elements == null) - { - return false; - } - - int dstSize = dst.size; - int expectedCapacity = dstSize + srcSize; + this.head.link = head = head.next; + } - if (expectedCapacity > dst.elements.Length) - { - int actualCapacity = dst.IncreaseCapacity(expectedCapacity); - srcEnd = Math.Min(srcStart + actualCapacity - dstSize, srcEnd); - } + int srcStart = head.ReadIndex; + int srcEnd = head.WriteIndex; + int srcSize = srcEnd - srcStart; + if (srcSize == 0) + { + return false; + } - if (srcStart != srcEnd) - { - DefaultHandle[] srcElems = head.elements; - DefaultHandle[] dstElems = dst.elements; - int newDstSize = dstSize; - if (head.elements == null) + if (dst?.elements == null) { return false; } - - for (int i = srcStart; i < srcEnd; i++) + + int dstSize = dst.size; + int expectedCapacity = dstSize + srcSize; + + if (expectedCapacity > dst.elements.Length) { - DefaultHandle element = srcElems[i]; - if (element == null) + int actualCapacity = dst.IncreaseCapacity(expectedCapacity); + srcEnd = Math.Min(srcStart + actualCapacity - dstSize, srcEnd); + } + + if (srcStart != srcEnd) + { + DefaultHandle[] srcElems = head.elements; + DefaultHandle[] dstElems = dst.elements; + int newDstSize = dstSize; + if (head.elements == null) { return false; } - - if (element.recycleId == 0) + + for (int i = srcStart; i < srcEnd; i++) { - element.recycleId = element.lastRecycledId; + DefaultHandle element = srcElems[i]; + if (element == null) + { + return false; + } + + if (element.recycleId == 0) + { + element.recycleId = element.lastRecycledId; + } + else if (element.recycleId != element.lastRecycledId) + { + throw new InvalidOperationException("recycled already"); + } + + srcElems[i] = null; + + if (dst.DropHandle(element)) + { + // Drop the object. + continue; + } + + element.Stack = dst; + dstElems[newDstSize++] = element; } - else if (element.recycleId != element.lastRecycledId) + + if (srcEnd == LinkCapacity && head.next != null) { - throw new InvalidOperationException("recycled already"); + // Add capacity back as the Link is GCed. + this.head.ReclaimSpace(LinkCapacity); + this.head.link = head.next; } - srcElems[i] = null; - if (dst.DropHandle(element)) + head.ReadIndex = srcEnd; + if (dst.size == newDstSize) { - // Drop the object. - continue; + return false; } - element.Stack = dst; - dstElems[newDstSize++] = element; - } - if (srcEnd == LinkCapacity && head.next != null) - { - // Add capacity back as the Link is GCed. - this.head.ReclaimSpace(LinkCapacity); - this.head.link = head.next; + dst.size = newDstSize; + return true; } - - head.ReadIndex = srcEnd; - if (dst.size == newDstSize) + else { + // The destination stack is full already. return false; } - dst.size = newDstSize; - return true; } - else + catch (NullReferenceException) { - // The destination stack is full already. return false; } } From 0333dbd71a00ee0329b81ae19d7aac9fd93f3f7c Mon Sep 17 00:00:00 2001 From: Ruben Buniatyan Date: Thu, 29 Jun 2023 14:25:47 +0200 Subject: [PATCH 3/7] Update package version and feed --- .github/workflows/build.yml | 2 +- src/Directory.Build.props | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 47f5c749..57540175 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -42,4 +42,4 @@ jobs: if: ${{ inputs.publish }} run: | dotnet pack -c ${{ env.BUILD_CONFIG }} --no-build - dotnet nuget push build_output/packages/*.nupkg -k ${{ secrets.NUGET_API_KEY }} -s https://api.nuget.org/v3/index.json + dotnet nuget push build_output/packages/*.nupkg -k ${{ secrets.NUGETTEST_API_KEY }} -s https://apiint.nugettest.org/v3/index.json diff --git a/src/Directory.Build.props b/src/Directory.Build.props index c5e968a6..3c8a1fce 100644 --- a/src/Directory.Build.props +++ b/src/Directory.Build.props @@ -16,7 +16,7 @@ True true - 1.0.0 + 1.0.1-preview.1 $(PackageVersion) From 1c9d84e40239d6bb1345dc7dfaafbe70b8a46f4d Mon Sep 17 00:00:00 2001 From: MarekM25 Date: Tue, 4 Jul 2023 13:55:21 +0200 Subject: [PATCH 4/7] remove try catch --- src/DotNetty.Common/ThreadLocalPool.cs | 218 ++++++++++++++----------- 1 file changed, 127 insertions(+), 91 deletions(-) diff --git a/src/DotNetty.Common/ThreadLocalPool.cs b/src/DotNetty.Common/ThreadLocalPool.cs index 2ea2fc89..a643101c 100644 --- a/src/DotNetty.Common/ThreadLocalPool.cs +++ b/src/DotNetty.Common/ThreadLocalPool.cs @@ -56,6 +56,7 @@ public override void Release(T value) { throw new InvalidOperationException("recycled already"); } + stack.Push(this); } } @@ -107,6 +108,7 @@ internal Head(StrongBox availableSharedCapacity, StrongBox weakTableCo { Interlocked.Decrement(ref this.weakTableCounter.Value); } + if (this.availableSharedCapacity == null) { return; @@ -138,13 +140,14 @@ internal bool ReserveSpace(int space) internal static bool ReserveSpace(StrongBox availableSharedCapacity, int space) { Debug.Assert(space >= 0); - for (; ; ) + for (;;) { int available = Volatile.Read(ref availableSharedCapacity.Value); if (available < space) { return false; } + if (Interlocked.CompareExchange(ref availableSharedCapacity.Value, available - space, available) == available) { return true; @@ -155,7 +158,9 @@ internal static bool ReserveSpace(StrongBox availableSharedCapacity, int sp // chain of data items readonly Head head; + Link tail; + // pointer to another queue of delayed items for the same stack internal WeakOrderQueue next; internal readonly WeakReference owner; @@ -220,10 +225,12 @@ internal void Add(DefaultHandle handle) // Drop it. return; } + // We allocate a Link so reserve the space this.tail = tail = tail.next = new Link(); writeIndex = tail.WriteIndex; } + tail.elements[writeIndex] = handle; handle.Stack = null; // we lazy set to ensure that setting stack to null appears before we unnull it in the owning thread; @@ -236,109 +243,102 @@ internal void Add(DefaultHandle handle) // transfer as many items as we can from this queue to the stack, returning true if any were transferred internal bool Transfer(Stack dst) { - try + Link head = this.head?.link; + if (head == null) + { + return false; + } + + if (head.ReadIndex == LinkCapacity) { - Link head = this.head?.link; - if (head == null) + if (head.next == null) { return false; } - if (head.ReadIndex == LinkCapacity) - { - if (head.next == null) - { - return false; - } + this.head.link = head = head.next; + } - this.head.link = head = head.next; - } + int srcStart = head.ReadIndex; + int srcEnd = head.WriteIndex; + int srcSize = srcEnd - srcStart; + if (srcSize == 0) + { + return false; + } - int srcStart = head.ReadIndex; - int srcEnd = head.WriteIndex; - int srcSize = srcEnd - srcStart; - if (srcSize == 0) - { - return false; - } + if (dst?.elements == null) + { + return false; + } - if (dst?.elements == null) - { - return false; - } + int dstSize = dst.size; + int expectedCapacity = dstSize + srcSize; - int dstSize = dst.size; - int expectedCapacity = dstSize + srcSize; + if (expectedCapacity > dst.elements.Length) + { + int actualCapacity = dst.IncreaseCapacity(expectedCapacity); + srcEnd = Math.Min(srcStart + actualCapacity - dstSize, srcEnd); + } - if (expectedCapacity > dst.elements.Length) + if (srcStart != srcEnd) + { + DefaultHandle[] srcElems = head.elements; + DefaultHandle[] dstElems = dst.elements; + int newDstSize = dstSize; + if (head.elements == null) { - int actualCapacity = dst.IncreaseCapacity(expectedCapacity); - srcEnd = Math.Min(srcStart + actualCapacity - dstSize, srcEnd); + return false; } - if (srcStart != srcEnd) + for (int i = srcStart; i < srcEnd; i++) { - DefaultHandle[] srcElems = head.elements; - DefaultHandle[] dstElems = dst.elements; - int newDstSize = dstSize; - if (head.elements == null) + DefaultHandle element = srcElems[i]; + if (element == null) { return false; } - for (int i = srcStart; i < srcEnd; i++) + if (element.recycleId == 0) { - DefaultHandle element = srcElems[i]; - if (element == null) - { - return false; - } - - if (element.recycleId == 0) - { - element.recycleId = element.lastRecycledId; - } - else if (element.recycleId != element.lastRecycledId) - { - throw new InvalidOperationException("recycled already"); - } - - srcElems[i] = null; - - if (dst.DropHandle(element)) - { - // Drop the object. - continue; - } - - element.Stack = dst; - dstElems[newDstSize++] = element; + element.recycleId = element.lastRecycledId; } - - if (srcEnd == LinkCapacity && head.next != null) + else if (element.recycleId != element.lastRecycledId) { - // Add capacity back as the Link is GCed. - this.head.ReclaimSpace(LinkCapacity); - this.head.link = head.next; + throw new InvalidOperationException("recycled already"); } - head.ReadIndex = srcEnd; - if (dst.size == newDstSize) + srcElems[i] = null; + + if (dst.DropHandle(element)) { - return false; + // Drop the object. + continue; } - dst.size = newDstSize; - return true; + element.Stack = dst; + dstElems[newDstSize++] = element; } - else + + if (srcEnd == LinkCapacity && head.next != null) + { + // Add capacity back as the Link is GCed. + this.head.ReclaimSpace(LinkCapacity); + this.head.link = head.next; + } + + head.ReadIndex = srcEnd; + if (dst.size == newDstSize) { - // The destination stack is full already. return false; } + + dst.size = newDstSize; + return true; } - catch (NullReferenceException) + else { + // The destination stack is full already. return false; } } @@ -370,8 +370,13 @@ protected sealed class Stack WeakOrderQueue cursorQueue, prevQueue; volatile WeakOrderQueue headQueue; - internal Stack(ThreadLocalPool parent, Thread thread, int maxCapacity, int maxSharedCapacityFactor, - int ratioMask, int maxDelayedQueues) + internal Stack( + ThreadLocalPool parent, + Thread thread, + int maxCapacity, + int maxSharedCapacityFactor, + int ratioMask, + int maxDelayedQueues) { this.parent = parent; this.threadRef = new WeakReference(thread); @@ -436,6 +441,7 @@ void PushNow(DefaultHandle item) { throw new InvalidOperationException("released already"); } + item.recycleId = item.lastRecycledId = ownThreadId; int size = this.size; @@ -444,6 +450,7 @@ void PushNow(DefaultHandle item) // Hit the maximum capacity - drop the possibly youngest object. return; } + if (size == this.elements.Length) { Array.Resize(ref this.elements, Math.Min(size << 1, this.maxCapacity)); @@ -469,12 +476,14 @@ void PushLater(DefaultHandle item, Thread thread) delayedRecycled.Add(this, WeakOrderQueue.Dummy); return; } + // Check if we already reached the maximum number of delayed queues and if we can allocate at all. if ((queue = WeakOrderQueue.Allocate(this, thread, countedWeakTable)) == null) { // drop object return; } + delayedRecycled.Add(this, queue); } else if (queue == WeakOrderQueue.Dummy) @@ -495,8 +504,10 @@ internal bool DropHandle(DefaultHandle handle) // Drop the object. return true; } + handle.hasBeenRecycled = true; } + return false; } @@ -512,8 +523,10 @@ internal bool TryPop(out DefaultHandle item) item = null; return false; } + size = this.size; } + size--; DefaultHandle ret = this.elements[size]; elements[size] = null; @@ -521,6 +534,7 @@ internal bool TryPop(out DefaultHandle item) { throw new InvalidOperationException("recycled multiple times"); } + ret.recycleId = 0; ret.lastRecycledId = 0; this.size = size; @@ -590,6 +604,7 @@ bool ScavengeSome() } } } + if (prev != null) { prev.Next = next; @@ -630,6 +645,7 @@ public class CountedWeakTable internal readonly StrongBox Counter = new StrongBox(); } + protected override CountedWeakTable GetInitialValue() => new CountedWeakTable(); } @@ -638,8 +654,9 @@ static ThreadLocalPool() // In the future, we might have different maxCapacity for different object types. // e.g. io.netty.recycler.maxCapacity.writeTask // io.netty.recycler.maxCapacity.outboundBuffer - int maxCapacityPerThread = SystemPropertyUtil.GetInt("io.netty.recycler.maxCapacityPerThread", - SystemPropertyUtil.GetInt("io.netty.recycler.maxCapacity", DefaultInitialMaxCapacityPerThread)); + int maxCapacityPerThread = SystemPropertyUtil.GetInt( + "io.netty.recycler.maxCapacityPerThread", + SystemPropertyUtil.GetInt("io.netty.recycler.maxCapacity", DefaultInitialMaxCapacityPerThread)); if (maxCapacityPerThread < 0) { maxCapacityPerThread = DefaultInitialMaxCapacityPerThread; @@ -647,17 +664,21 @@ static ThreadLocalPool() DefaultMaxCapacityPerThread = maxCapacityPerThread; - DefaultMaxSharedCapacityFactor = Math.Max(2, - SystemPropertyUtil.GetInt("io.netty.recycler.maxSharedCapacityFactor", - 2)); + DefaultMaxSharedCapacityFactor = Math.Max( + 2, + SystemPropertyUtil.GetInt( + "io.netty.recycler.maxSharedCapacityFactor", + 2)); - DefaultMaxDelayedQueuesPerThread = Math.Max(0, - SystemPropertyUtil.GetInt("io.netty.recycler.maxDelayedQueuesPerThread", - // We use the same value as default EventLoop number - Environment.ProcessorCount * 2)); + DefaultMaxDelayedQueuesPerThread = Math.Max( + 0, + SystemPropertyUtil.GetInt( + "io.netty.recycler.maxDelayedQueuesPerThread", + // We use the same value as default EventLoop number + Environment.ProcessorCount * 2)); LinkCapacity = MathUtil.SafeFindNextPositivePowerOfTwo( - Math.Max(SystemPropertyUtil.GetInt("io.netty.recycler.linkCapacity", 16), 16)); + Math.Max(SystemPropertyUtil.GetInt("io.netty.recycler.linkCapacity", 16), 16)); // By default we allow one push to a Recycler for each 8th try on handles that were never recycled before. // This should help to slowly increase the capacity of the recycler while not be too sensitive to allocation @@ -689,12 +710,15 @@ static ThreadLocalPool() } public ThreadLocalPool(int maxCapacityPerThread) - : this (maxCapacityPerThread, DefaultMaxSharedCapacityFactor, DefaultRatio, DefaultMaxDelayedQueuesPerThread) + : this(maxCapacityPerThread, DefaultMaxSharedCapacityFactor, DefaultRatio, DefaultMaxDelayedQueuesPerThread) { } - public ThreadLocalPool(int maxCapacityPerThread, int maxSharedCapacityFactor, - int ratio, int maxDelayedQueuesPerThread) + public ThreadLocalPool( + int maxCapacityPerThread, + int maxSharedCapacityFactor, + int ratio, + int maxDelayedQueuesPerThread) { this.ratioMask = MathUtil.SafeFindNextPositivePowerOfTwo(ratio) - 1; if (maxCapacityPerThread <= 0) @@ -744,8 +768,13 @@ public ThreadLocalPool(Func valueFactory, int maxCapacityPerThread, i { } - public ThreadLocalPool(Func valueFactory, int maxCapacityPerThread, int maxSharedCapacityFactor, - int ratio, int maxDelayedQueuesPerThread, bool preCreate = false) + public ThreadLocalPool( + Func valueFactory, + int maxCapacityPerThread, + int maxSharedCapacityFactor, + int ratio, + int maxDelayedQueuesPerThread, + bool preCreate = false) : base(maxCapacityPerThread, maxSharedCapacityFactor, ratio, maxDelayedQueuesPerThread) { Contract.Requires(valueFactory != null); @@ -769,6 +798,7 @@ public T Take() { handle = CreateValue(stack); } + return (T)handle.Value; } @@ -794,8 +824,13 @@ public ThreadLocalStack(ThreadLocalPool owner) protected override Stack GetInitialValue() { - var stack = new Stack(this.owner, Thread.CurrentThread, this.owner.maxCapacityPerThread, - this.owner.maxSharedCapacityFactor, this.owner.ratioMask, this.owner.maxDelayedQueuesPerThread); + var stack = new Stack( + this.owner, + Thread.CurrentThread, + this.owner.maxCapacityPerThread, + this.owner.maxSharedCapacityFactor, + this.owner.ratioMask, + this.owner.maxDelayedQueuesPerThread); if (this.owner.preCreate) { for (int i = 0; i < this.owner.maxCapacityPerThread; i++) @@ -803,6 +838,7 @@ protected override Stack GetInitialValue() stack.Push(this.owner.CreateValue(stack)); } } + return stack; } From 57440534dfbcc47c86b759c92dcdaffd7cf24053 Mon Sep 17 00:00:00 2001 From: Ruben Buniatyan Date: Tue, 4 Jul 2023 14:01:18 +0200 Subject: [PATCH 5/7] Bump up the version --- src/Directory.Build.props | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Directory.Build.props b/src/Directory.Build.props index 3c8a1fce..c27273d1 100644 --- a/src/Directory.Build.props +++ b/src/Directory.Build.props @@ -16,7 +16,7 @@ True true - 1.0.1-preview.1 + 1.0.1-preview.2 $(PackageVersion) From 2d5a39a895fd401a9b862b069ae194b49cd4ccf6 Mon Sep 17 00:00:00 2001 From: Ruben Buniatyan Date: Mon, 31 Jul 2023 16:21:56 +0200 Subject: [PATCH 6/7] Remove pre-release designator --- src/Directory.Build.props | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Directory.Build.props b/src/Directory.Build.props index c27273d1..a250448a 100644 --- a/src/Directory.Build.props +++ b/src/Directory.Build.props @@ -16,7 +16,7 @@ True true - 1.0.1-preview.2 + 1.0.1 $(PackageVersion) From 8054b28c0e56eef21efa102d5ad2797cad5fda6d Mon Sep 17 00:00:00 2001 From: Ruben Buniatyan Date: Mon, 31 Jul 2023 16:23:34 +0200 Subject: [PATCH 7/7] Revert publishing feed --- .github/workflows/build.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 57540175..47f5c749 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -42,4 +42,4 @@ jobs: if: ${{ inputs.publish }} run: | dotnet pack -c ${{ env.BUILD_CONFIG }} --no-build - dotnet nuget push build_output/packages/*.nupkg -k ${{ secrets.NUGETTEST_API_KEY }} -s https://apiint.nugettest.org/v3/index.json + dotnet nuget push build_output/packages/*.nupkg -k ${{ secrets.NUGET_API_KEY }} -s https://api.nuget.org/v3/index.json