Once approved I will start the process of backporting it to 3.1 and 5.0.

Can we add some regression tests for this?

Updated with feedback and unit tests.
I also used the repro code provided with the issue to confirm the bug, then tried these changes to confirm it was fixing it.

Wouldn't it be simpler to support a null fake memory owner and just use ArrayPool.Shared?

Wouldn't it be simpler to support a null fake memory owner and just use ArrayPool.Shared?

What's the benefit? Wouldn't you need to special case returning the array in Dispose() in that case?

I would understand @davidfowl 's suggestion as replacing _fakeMemoryOwner by byte[] _fakeMemory which would always come from the ArrayPool.Shared if set, and then returned in Dispose. So as single source. But I don't know why it was using _memoryPool before vs. the array pool.

What's the benefit? Wouldn't you need to special case returning the array in Dispose() in that case?

This code originally came from https://source.dot.net/#System.IO.Pipelines/System/IO/Pipelines/BufferSegment.cs,56 which handles this in a much nicer way IMO.

What's nicer about BufferSegment's way of doing it?

It's isolated in the buffer segment and doesn't allocate another kind of IMemoryOwner<byte>.

I am trying to implement what I think you meant ... hope it's worth it. Will do it in a separate branch as I am very not sure about the end result.

@davidfowl is this what you meant?

5de377a

@davidfowl is this what you meant?

I'll try channeling @davidfowl. Allocating a FakeMemory isn't any better than allocating "another kind of IMemoryOwner<byte>.". This allocates even more than before in the case where the _memoryPool is used in FakeMemory.

If FakeMemory was pooled like BufferSegments are in System.IO.Pipelines, this wouldn't introduce a new allocation, but that's not the case here. We shouldn't bother pooling FakeMemory since this would only get allocated when a connection/request is aborted meaning the output producer itself will never be pooled at this point.

Instead the suggestion is to put separate IMemoryOwner<byte>? and byte[]? fields diretly in the output producers that are never both set at the same time similar to BufferSegment.

Now I'm going to speak as myself again. This seems like a lot of work to avoid a single extra allocation for aborted requests where the writer needs large buffers. This is a pretty rare scenario that isn't going to be very efficient to begin with. That said, it's not that much extra work I guess.

@halter73 now I understand the comment, thanks.

updated

@davidfowl are the changes what you expected?

/backport to release/6.0

Started backporting to release/6.0: https://github.com/dotnet/aspnetcore/actions/runs/1211460440

/backport to release/5.0

Started backporting to release/5.0: https://github.com/dotnet/aspnetcore/actions/runs/1238671987

@sebastienros backporting to release/5.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Use ArrayPool as SlabMemoryPool fallback
Applying: Fix comments
Applying: Typos
Applying: Support increase buffer size
Applying: Add function tests, rename argument
error: sha1 information is lacking or useless (src/Servers/Kestrel/Core/src/Internal/Http/Http1OutputProducer.cs).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0005 Add function tests, rename argument
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!