Fix semantics of ArrayMemoryPool #27615

pakrym · 2018-03-01T19:02:12Z

https://github.com/dotnet/corefx/issues/27544
https://github.com/dotnet/corefx/issues/27543

Make ArrayMemoryPool block ref counting thread-safe.
ArrayMemoryPool block returned from the pool has ref count == 1
Releasing ArrayMemoryPool block causes it to be disposed and returned to the pool.

stephentoub · 2018-03-01T20:13:08Z

src/System.Memory/src/System/Buffers/ArrayMemoryPool.ArrayMemoryPoolBuffer.cs

            }

            public sealed override bool Release()
            {
                if (IsDisposed)
                    ThrowHelper.ThrowObjectDisposedException_ArrayMemoryPoolBuffer();

-                int newRefCount = --_refCount;
+                int newRefCount = Interlocked.Decrement(ref _refCount);


If you actually want thread-safety, these Interlocked.Decrement/Increments should be changed to CompareExchange loops, e.g.

// in Retain while (true) { int currentCount = Volatile.Read(ref _refCount); if (currentCount <= 0) ThrowHelper.ThrowObjectDisposedException_ArrayMemoryPoolBuffer(); if (Interlocked.CompareExchange(ref _refCount, currentCount + 1, currentCount) == currentCount) break; } ... // in Release while (true) { int currentCount = Volatile.Read(ref _refCount); if (currentCount <= 0) ThrowHelper.ThrowObjectDisposedException_ArrayMemoryPoolBuffer(); if (Interlocked.CompareExchange(ref _refCount, currentCount - 1, currentCount) == currentCount) { if (currentCount == 1) { Dispose(); return true; } return false; } }

Otherwise, consider this:

The current _refCount is 1.

Thread A calls Retain and gets past the IsDisposed check.

Thread B calls Release, disposing the object, and drops the count to 0.

Thread A resumes, increments the _refCount, and proceeds to party on the array thinking it successfully owns it.

Lots of other threads come along, and all Retain/Release, and as the ref count is now positive again, they're all successful.

@atsushikan could you look at adding thread safety tests here?

stephentoub · 2018-03-01T20:13:49Z

src/System.Memory/src/System/Buffers/ArrayMemoryPool.cs

@@ -22,7 +22,9 @@ public sealed override OwnedMemory<T> Rent(int minimumBufferSize = -1)
            else if (((uint)minimumBufferSize) > s_maxBufferSize)
                ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.minimumBufferSize);

-            return new ArrayMemoryPoolBuffer(minimumBufferSize);
+            var buffer = new ArrayMemoryPoolBuffer(minimumBufferSize);
+            buffer.Retain();


This doesn't need to incur an interlocked operation, as it's not published yet.

Do we want to do this? It changes the contract quite a bit. Can you fix pipelines in the same PR?

I'll fix pipelines. It's just strange to get an OwnedMemory with 0 references.

Everyone has to agree on the semantics, today our pool in kestrel does not do this so it'll result in memory leaks...

I removed Retain and added dispose call to Pipelines.

davidfowl · 2018-03-01T22:25:48Z

src/System.IO.Pipelines/src/System/IO/Pipelines/BufferSegment.cs

@@ -75,6 +74,7 @@ public void SetMemory(OwnedMemory<byte> ownedMemory, int start, int end, bool re
        public void ResetMemory()
        {
            _ownedMemory.Release();
+            _ownedMemory.Dispose();


This isn't correct. We just need to release.

Are you sure? Pipe is the thing that got OM from the pool, so it is its responsibility to dispose it.

But even your PR descriptions says "Releasing ArrayMemoryPool block causes it to be disposed and returned to the pool." Having said that I do think it's a bit strange that we have both Release and Dispose on this type.

But calling dispose would also validate that there are no other owners left.

This is beneficial in the case of pipelines. Imagine a situation when someone reads ROS from the pipe, get's memory from it and calls Retain. Now when he tries to advance, Dispose would throw and avoid use after free bugs.

On the other hand, we can decide to allow consumers to Retain memory received from pipe and calling dispose here would break this feature.

I think we should just release and not dispose

davidfowl · 2018-03-01T22:26:13Z

src/System.Memory/src/System/Buffers/ArrayMemoryPool.cs

@@ -22,7 +22,8 @@ public sealed override OwnedMemory<T> Rent(int minimumBufferSize = -1)
            else if (((uint)minimumBufferSize) > s_maxBufferSize)
                ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.minimumBufferSize);

-            return new ArrayMemoryPoolBuffer(minimumBufferSize);
+            var buffer = new ArrayMemoryPoolBuffer(minimumBufferSize);


You can revert this piece.

davidfowl · 2018-03-01T22:30:17Z

Does disposing throw if the reference count is > 0?

pakrym · 2018-03-01T22:34:16Z

Yes

pakrym · 2018-03-01T22:38:59Z

This is a general question about OwnedMemory design: is something creates owned memory for you, should it be returned with ref count equal to one or zero?

KrzysztofCwalina · 2018-03-02T00:59:07Z

src/System.Memory/src/System/Buffers/ArrayMemoryPool.ArrayMemoryPoolBuffer.cs

            }

            public sealed override int Length => _array.Length;

            public sealed override bool IsDisposed => _array == null;

-            protected sealed override bool IsRetained => _refCount > 0;
+            protected sealed override bool IsRetained => Volatile.Read(ref _refCount) > 0;


@stephentoub, does it need to be interlocked read?

does it need to be interlocked read?

No, it's only 32-bit, so there's no risk of tearing and Interlocked.Read isn't needed. Volatile.Read may not even be necessary depending on what it's used for, but it's unlikely to hurt and so might as well be used.

Ah, I missed that it's an int.

pakrym · 2018-03-02T16:17:06Z

@KrzysztofCwalina why was dispose added to OwnedMemory? What was it's intended semantics?

KrzysztofCwalina · 2018-03-02T19:43:37Z

@KrzysztofCwalina why was dispose added to OwnedMemory? What was it's intended semantics?

Dispose is for "I own the memory; I am disposing it. If you (Memory) still try to access it, you will get an exception as there is a bug somewhere". Relying on just release opens the possibility of undetected leaks.

ahsonkhan · 2018-03-02T20:20:30Z

src/System.Memory/src/System/Buffers/ArrayMemoryPool.ArrayMemoryPoolBuffer.cs

@@ -3,6 +3,7 @@
 // See the LICENSE file in the project root for more information.

 using System.Runtime.InteropServices;
+using System.Threading;


nit: add new line

ahsonkhan · 2018-03-02T20:23:14Z

src/System.IO.Pipelines/tests/PipePoolTests.cs

@@ -3,6 +3,7 @@
 // See the LICENSE file in the project root for more information.

 using System.Buffers;
+using System.Diagnostics;


Do we need this using directive?

pakrym · 2018-03-02T20:36:43Z

@KrzysztofCwalina

Dispose is for "I own the memory; I am disposing it. If you (Memory) still try to access it, you will get an exception as there is a bug somewhere". Relying on just release opens the possibility of undetected leaks.

So when you get memory from the pool, does it mean that you own it and have to dispose?

davidfowl · 2018-03-03T16:29:54Z

The issue with disposing in pipelines is that you lose the ability for the pipe to transfer ownership to another pipe (Append). The original idea was that the reference count would just work because once you transfer the buffer to another pipe, it would just up the reference count and take over those segments.

pakrym · 2018-03-05T19:45:17Z

Okay, I removed dispose call from pipe but kept the part where OwnedMemory is returned with refCount == 1

davidfowl · 2018-03-06T05:09:41Z

@KrzysztofCwalina speak now or forever hold your peace. This change means that if you get an owned memory from a MemoryPool<T>.Rent, you have to call Release before Dispose.

davidfowl

LGTM

* Fix semantics of ArrayMemoryPool * More thread safety * Fix pipes and add tests Commit migrated from dotnet/corefx@9ce9033

pakrym requested review from davidfowl, stephentoub, KrzysztofCwalina and a user March 1, 2018 19:02

stephentoub reviewed Mar 1, 2018

View reviewed changes

davidfowl reviewed Mar 1, 2018

View reviewed changes

pakrym force-pushed the pakrym/memory-pool-samantics branch from 27e7b74 to 4eecbab Compare March 1, 2018 22:28

pakrym force-pushed the pakrym/memory-pool-samantics branch 2 times, most recently from 34779af to d5467b2 Compare March 2, 2018 00:04

KrzysztofCwalina reviewed Mar 2, 2018

View reviewed changes

ahsonkhan added the area-System.Memory label Mar 2, 2018

ahsonkhan assigned pakrym Mar 2, 2018

pakrym force-pushed the pakrym/memory-pool-samantics branch 3 times, most recently from 394b421 to efa461f Compare March 2, 2018 19:31

ahsonkhan reviewed Mar 2, 2018

View reviewed changes

pakrym force-pushed the pakrym/memory-pool-samantics branch 2 times, most recently from 4576877 to edcf2e5 Compare March 5, 2018 19:38

pakrym added 2 commits March 5, 2018 11:40

Fix semantics of ArrayMemoryPool

392ff92

More thread safety

0c7e2cf

Fix pipes and add tests

0843b65

pakrym force-pushed the pakrym/memory-pool-samantics branch from edcf2e5 to 0843b65 Compare March 5, 2018 19:42

davidfowl approved these changes Mar 6, 2018

View reviewed changes

ahsonkhan merged commit 9ce9033 into dotnet:master Mar 6, 2018

davidfowl mentioned this pull request Mar 6, 2018

React to MemoryPool<T> changes dotnet/aspnetcore#2923

Closed

karelz added this to the 2.1.0 milestone Mar 10, 2018

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Fix semantics of ArrayMemoryPool (dotnet/corefx#27615)

afe60bd

* Fix semantics of ArrayMemoryPool * More thread safety * Fix pipes and add tests Commit migrated from dotnet/corefx@9ce9033

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix semantics of ArrayMemoryPool #27615

Fix semantics of ArrayMemoryPool #27615

pakrym commented Mar 1, 2018 •

edited

Loading

stephentoub Mar 1, 2018 •

edited

Loading

joshfree Mar 1, 2018

stephentoub Mar 1, 2018

davidfowl Mar 1, 2018

pakrym Mar 1, 2018

davidfowl Mar 1, 2018

pakrym Mar 1, 2018

davidfowl Mar 1, 2018

pakrym Mar 1, 2018

KrzysztofCwalina Mar 2, 2018 •

edited

Loading

pakrym Mar 2, 2018

pakrym Mar 2, 2018

davidfowl Mar 2, 2018

davidfowl Mar 1, 2018

davidfowl commented Mar 1, 2018

pakrym commented Mar 1, 2018

pakrym commented Mar 1, 2018

KrzysztofCwalina Mar 2, 2018

stephentoub Mar 2, 2018

KrzysztofCwalina Mar 2, 2018

pakrym commented Mar 2, 2018

KrzysztofCwalina commented Mar 2, 2018 •

edited

Loading

ahsonkhan Mar 2, 2018

ahsonkhan Mar 2, 2018

pakrym commented Mar 2, 2018

davidfowl commented Mar 3, 2018

pakrym commented Mar 5, 2018

davidfowl commented Mar 6, 2018

davidfowl left a comment

Fix semantics of ArrayMemoryPool #27615

Fix semantics of ArrayMemoryPool #27615

Conversation

pakrym commented Mar 1, 2018 • edited Loading

stephentoub Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrzysztofCwalina Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl commented Mar 1, 2018

pakrym commented Mar 1, 2018

pakrym commented Mar 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakrym commented Mar 2, 2018

KrzysztofCwalina commented Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pakrym commented Mar 2, 2018

davidfowl commented Mar 3, 2018

pakrym commented Mar 5, 2018

davidfowl commented Mar 6, 2018

davidfowl left a comment

Choose a reason for hiding this comment

pakrym commented Mar 1, 2018 •

edited

Loading

stephentoub Mar 1, 2018 •

edited

Loading

KrzysztofCwalina Mar 2, 2018 •

edited

Loading

KrzysztofCwalina commented Mar 2, 2018 •

edited

Loading