Make PooledByteBufferWriter public

[blankensteiner]

Hi
I'm writing my own serializer using Utf8JsonWriter and Utf8JsonReader.
I have performance tested:

• Utf8JsonWriter.Utf8JsonWriter(Stream utf8Json, JsonWriterOptions options = default) with a MemoryStream.
• Utf8JsonWriter.Utf8JsonWriter(IBufferWriter bufferWriter, JsonWriterOptions options = default) with an ArrayBufferWriter.
• Utf8JsonWriter.Utf8JsonWriter(IBufferWriter bufferWriter, JsonWriterOptions options = default) with a PooledByteBufferWriter.

The latter outperforms the first two and it would be awesome if it could be made public (it's internal now). I noticed that once ArrayBufferWriter was internal, but was made public, so I'm hoping the same thing could happen here :-)

[GrabYourPitchforks]

The libraries team isn't yet exposing "dangerous" types like this that use ArrayPool under the covers. The reason for this is that it's extremely easy to misuse these types, which could lead to global memory corruption of the application and would be nearly impossible to debug. This would do more harm than good to the ecosystem at large without a language feature that can enforce proper usage of these types.

Think of it as conceptually similar to the below code, where the compiler enforces lifetime management of the spans to prevent the application from encountering strange failures at runtime.

public static Span<byte> GetSpanFoo()
{
    Span<byte> theSpan = new byte[100];
    return theSpan; // ok
}

public static Span<byte> GetSpanBar()
{
    Span<byte> theSpan = stackalloc byte[100];
    return theSpan; // compiler fails on this line
}

See also #25587, where the proposed ValueStringBuilder suffers from a similar problem.

[blankensteiner]

Hi @GrabYourPitchforks
I see. Maybe we can find an alternative then since it would be a shame if only JsonSerializer can make Utf8JsonWriter use it.
I've seen other teams at Microsoft that places stuff in an "Internal" namespace, but make them public. Could that be a solution? Moving PooledByteBufferWriter to 'System.Text.Json.Internal' and thereby signaling that it's meant for internal use and therefore could be subject to change and/or "dangerous" to use?

[blankensteiner]

E.g https://github.com/dotnet/efcore/blob/master/src/EFCore/Internal/AsyncLock.cs

[layomia]

Moving to System.Memory since it is a general purpose ask related to buffers/IBufferWriter.

[davidfowl]

I 100% agree. We were overly cautious with ArrayBufferWriter but we avoid using it internally because we want to be efficient. I think it should be possible to use the ArrayBufferWriter with pooled buffers.

[MithrilMan]

I feel the need of a PooledByteBuffer too as stated in #40007
I understand the concern of misuse, then why don't hide the behavior behind a constructor overload ArrayBufferWriter(bool usePool)? (semi joke)
BTW I don't feel the exmple of Span fitting the ArrayPool problem (Maybe I'm underestimating some problem, I can just think about misusing PooledByteBuffer as a long living object not returning rented memory cause the pool to be constant out of memory causing it to keep allocating memory out of the memory pool)

[GrabYourPitchforks]

I understand the concern of misuse, then why don't hide the behavior behind a constructor overload

This isn't the typical pattern used by the .NET team.

Here's a better analogy: we don't often make thread safety guarantees with many of our types. For example, if you have two threads operating on a List<T> concurrently, they're going to cause data corruption since we don't make any thread safety guarantees on that type. But the important distinction is that they corrupt data only within that one List<T> instance. Other List<T> instances in use by your app will be just fine.

With PooledByteBufferWriter (this issue) and ValueStringBuilder (see #25587), misuse of the API is potentially fatal to the application. Two threads accidentally touch a single PooledByteBufferWriter? Accidentally make a copy-by-value of a ValueStringBuilder? You've now inadvertently returned the same array to the array pool twice. This can lead to all sorts of problems, from the annoying (exception messages don't display correctly) to the disastrous (your web service starts sending responses to the wrong users).

The reason we generally don't like using constructor overloads for this is that the entity who makes the safety decision (whoever called the new ctor) and the entity who consumes the buffer writer are often in different libraries. We don't like being in a situation where somebody tests against one implementation and believes that things just work, and then they're handed a different implementation and things fall over in unexpected and unpredictable ways. That's why for both this issue and for #25587 we're brainstorming some kind of compiler or runtime support that would enforce correct behavior on the part of the consumers. If we can get that working, then the concerns about passing these types around dissipates quite a bit.

[osexpert]

Maybe it should be called PooledArrayBufferWriter instead of PooledByteBufferWriter?
You could be forced to provide a private ArrayPool in the ctor to avoid global memory corruption (just like RecyclableMemoryStream has RecyclableMemoryStreamManager, and manager is the private pool)