Change exception for overflow in ArrayBufferWriter

[felipepessoto]

Continuation from #1308

[felipepessoto]

https://github.com/dotnet/runtime/blob/9505b7314f291e3788cb5e557b0d62e4c1103433/src/libraries/Common/src/System/Buffers/ArrayBufferWriter.cs#L173-L176

This (pre-existing) if is not clear to me. If the buffer.Length is 0, like when we just created the instance. It doesn't respect the sizeHint

[felipepessoto]

The test is failing in "netcoreapp5.0-OSX-Debug-x64-Mono_release-OSX.1013.Amd64.Open" environment.
@ahsonkhan, do you know what can cause this difference?

[ahsonkhan]

I am not sure why that is failing, but it looks like it is mono specific:
https://helix.dot.net/api/2019-06-17/jobs/d839f076-ff49-437f-ae7b-77ff86928be6/workitems/System.Memory.Tests/console

Starting:    System.Memory.Tests (parallel test collections = on, max threads = 4)
    System.Buffers.Tests.ArrayBufferWriterTests_String.GetMemory_ExceedMaximumBufferSize [FAIL]
      Assert.Throws() Failure
      Expected: typeof(System.OutOfMemoryException)
      Actual:   (No exception was thrown)
      Stack Trace:
        /_/src/libraries/System.Memory/tests/ArrayBufferWriter/ArrayBufferWriterTests.T.cs(217,0): at System.Buffers.Tests.ArrayBufferWriterTests`1[[System.String, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].GetMemory_ExceedMaximumBufferSize()
        /_/src/mono/netcore/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs(339,0): at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
    System.Buffers.Tests.ArrayBufferWriterTests_Byte.GetMemory_ExceedMaximumBufferSize [FAIL]
      Assert.Throws() Failure
      Expected: typeof(System.OutOfMemoryException)
      Actual:   (No exception was thrown)
      Stack Trace:
        /_/src/libraries/System.Memory/tests/ArrayBufferWriter/ArrayBufferWriterTests.T.cs(217,0): at System.Buffers.Tests.ArrayBufferWriterTests`1[[System.Byte, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].GetMemory_ExceedMaximumBufferSize()
        /_/src/mono/netcore/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs(339,0): at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
    System.Buffers.Tests.ArrayBufferWriterTests_Char.GetMemory_ExceedMaximumBufferSize [FAIL]
      Assert.Throws() Failure
      Expected: typeof(System.OutOfMemoryException)
      Actual:   (No exception was thrown)
      Stack Trace:
        /_/src/libraries/System.Memory/tests/ArrayBufferWriter/ArrayBufferWriterTests.T.cs(217,0): at System.Buffers.Tests.ArrayBufferWriterTests`1[[System.Char, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].GetMemory_ExceedMaximumBufferSize()
        /_/src/mono/netcore/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs(339,0): at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
  Finished:    System.Memory.Tests

@jkotas - do you know why we aren't throwing OOM on mono OSX here? Does it support more than int.MaxValue elements somehow?

var buffer = new byte[10];
Array.Resize<byte>(ref buffer, int.MaxValue);

cc @alexischr

[jkotas]

do you know why we aren't throwing OOM on mono OSX here? Does it support more than int.MaxValue elements somehow?

Look for PlatformDetection.IsNotIntMaxValueArrayIndexSupported

[ahsonkhan]

https://github.com/dotnet/runtime/blob/9505b7314f291e3788cb5e557b0d62e4c1103433/src/libraries/Common/src/System/Buffers/ArrayBufferWriter.cs#L173-L176

This (pre-existing) if is not clear to me. If the buffer.Length is 0, like when we just created the instance. It doesn't respect the sizeHint

The IBufferWriter<T> interface requires that we return a buffer that is at least as big as sizeHint. We can return something bigger if that is most optimal:
https://docs.microsoft.com/en-us/dotnet/api/system.buffers.ibufferwriter-1.getspan?view=netcore-3.1#System_Buffers_IBufferWriter_1_GetSpan_System_Int32_

In this case, if a user keeps asking for a buffer passing sizeHint = 0, it doesn't make sense to start the "doubling" with 1 (then 2, 4, 8, ...). So, we cap the lower bound by 256, which seems to be a reasonable starting size for most operations.

[felipepessoto]

do you know why we aren't throwing OOM on mono OSX here? Does it support more than int.MaxValue elements somehow?

Look for PlatformDetection.IsNotIntMaxValueArrayIndexSupported

@ahsonkhan
In this case, should we explicitly throw an OutOfMemory, instead of setting newSize to int.MaxValue? (I understand we only do that because of its side-effect is to throw an OOM Exception when we call Array.Resize. We may also need to do the same in my previous PR).

If we keep it the way it is now, in OSX Mono environment that method will return a Buffer of int.MaxValue size, instead of the size required, breaking the contract.

[ahsonkhan]

Look for PlatformDetection.IsNotIntMaxValueArrayIndexSupported

Yep, looks like we need to disable it for platforms that support arrays larger than int.Max:
mono/mono#15189 (comment)

[ahsonkhan]

If we keep it the way it is now, in OSX Mono environment that method will return a Buffer of int.MaxValue size, instead of the size required, breaking the contract.

Sure, that makes sense. I am not sure what we generally do in the rest of the framework for this type of discrepancy, but given I was leaning towards having a custom exception message anyway, this works (#1308 (comment)). Consider adding and calling a throwhelper method similar to:
https://github.com/dotnet/runtime/blob/d515c203410a50b2725400c969566c9f484a01ee/src/libraries/Common/src/System/Buffers/ArrayBufferWriter.cs#L195-L198

I am going to defer to what others have to say about that as the final decision though.

cc @stephentoub

[felipepessoto]

@stephentoub, should we explicitly throw OOM? And change my previous PR (#1308) to do the same?

Thanks

[ahsonkhan]

should we explicitly throw OOM?

For unblocking this change/PR, let's do it. I doubt we'll need to, but we can re-visit that decision later.

[felipepessoto]

This test (https://github.com/dotnet/runtime/pull/32587/checks?check_run_id=530741887) is failing:

   at System.Buffers.ArrayBufferWriter`1[[System.String, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]]..ctor(Int32 initialCapacity) in /_/src/libraries/Common/src/System/Buffers/ArrayBufferWriter.cs:line 47
   at System.Buffers.Tests.ArrayBufferWriterTests`1[[System.String, System.Private.CoreLib, Version=5.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].GetMemory_ExceedMaximumBufferSize() in /_/src/libraries/System.Memory/tests/ArrayBufferWriter/ArrayBufferWriterTests.T.cs:line 211
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture) in /_/src/mono/netcore/System.Private.CoreLib/src/System/Reflection/RuntimeMethodInfo.cs:line 339

after I added this line in the unit test:

new ArrayBufferWriter<T>(int.MaxValue / 2 + 1);

I'll restrict it to 64bits environment.

[felipepessoto]

And change my previous PR (#1308) to do the same?

Yes please ;)

Done: #34040
The build is failing, but it seems an existing problem

[ahsonkhan]

Some test suggestions.

[ahsonkhan]

I'll restrict it to 64bits environment.

That alone won't help. I am getting OOM even on my local machine. The test is taking up 12+ GB of memory to run (even if we don't call output.GetMemory(int.MaxValue)). Even for outerloop that's too much.
Please re-write it to not be so resource intensive.

The problem is the ArrayBufferWriterTests_String class is allocating a lot (~16 GB).

Rather than adding the test to the generic base class, make it specific to byte only and just add it to the ArrayBufferWriterTests_Byte tests (and remove it from the generic test class). That way you only allocate ~2 GB.

runtime/src/libraries/System.Memory/tests/ArrayBufferWriter/ArrayBufferWriterTests.Byte.cs

Lines 11 to 12 in 4040e60

	public class ArrayBufferWriterTests_Byte : ArrayBufferWriterTests<byte>
	{

// NOTE: GetMemory_ExceedMaximumBufferSize test is constrained to run on Windows and MacOSX because it causes
//       problems on Linux due to the way deferred memory allocation works. On Linux, the allocation can
//       succeed even if there is not enough memory but then the test may get killed by the OOM killer at the
//       time the memory is accessed which triggers the full memory allocation.
[PlatformSpecific(TestPlatforms.Windows | TestPlatforms.OSX)]
[ConditionalFact(nameof(IsX64))]
[OuterLoop]
public void GetMemory_ExceedMaximumBufferSize()
{
    var output = new ArrayBufferWriter<byte>(int.MaxValue / 2 + 1);
    output.Advance(int.MaxValue / 2 + 1);
    Memory<byte> memory = output.GetMemory(1); // Validate we can't double the buffer size, but can grow by sizeHint
    Assert.Equal(1, memory.Length);
    Assert.Throws<OutOfMemoryException>(() => output.GetMemory(int.MaxValue));
}

[ahsonkhan]

CI failure is unrelated: #34087