From 5ab6d641f1f866d364f15001388ae9546ffe6cb1 Mon Sep 17 00:00:00 2001 From: Grant Lodge <6323995+thelonelyvulpes@users.noreply.github.com> Date: Wed, 3 Jan 2024 16:42:18 +0000 Subject: [PATCH 1/8] Update array pool config, allow driver to request a max sized array --- .../Util/PipeReaderMemoryPoolTests.cs | 104 ++++++++++++++++++ .../Internal/Util/PipeReaderMemoryPool.cs | 2 +- 2 files changed, 105 insertions(+), 1 deletion(-) create mode 100644 Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/PipeReaderMemoryPoolTests.cs diff --git a/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/PipeReaderMemoryPoolTests.cs b/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/PipeReaderMemoryPoolTests.cs new file mode 100644 index 000000000..88b40fb86 --- /dev/null +++ b/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/PipeReaderMemoryPoolTests.cs @@ -0,0 +1,104 @@ +// Copyright (c) "Neo4j" +// Neo4j Sweden AB [https://neo4j.com] +// +// Licensed under the Apache License, Version 2.0 (the "License"). +// You may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +using System.Buffers; +using FluentAssertions; +using Xunit; + +namespace Neo4j.Driver.Internal.Util; + +public class PipeReaderMemoryPoolTests +{ + [Theory] + [InlineData(512, 512)] + [InlineData(1024, 1024)] + [InlineData(1025, 2048)] + [InlineData(2049, 4096)] + [InlineData(4096, 4096)] + [InlineData(4097, 8192)] + [InlineData(65535, 65536)] + [InlineData(65539, 131072)] + public void ShouldRentMemoryInPower(int size, int expectedSize) + { + var pool = new PipeReaderMemoryPool(1024); + using var memory = pool.Rent(size); + memory.Memory.Length.Should().Be(expectedSize); + } + + [Fact] + public void ShouldReusePooledObjects() + { + var pool = new PipeReaderMemoryPool(1024); + + int length; + using (var memory = pool.Rent(4321)) + { + length = memory.Memory.Length; + memory.Memory.Span[0] = 1; + } + + using (var memory = pool.Rent(4321)) + { + memory.Memory.Length.Should().Be(length); + memory.Memory.Span[0].Should().Be(1); + } + } + + [Fact] + public void SharedPoolShouldReturnSameValue() + { + var pool = MemoryPool.Shared; + int length; + using (var memory = pool.Rent(4321)) + { + length = memory.Memory.Length; + memory.Memory.Span[0] = 1; + } + + using (var memory = pool.Rent(4321)) + { + memory.Memory.Length.Should().Be(length); + memory.Memory.Span[0].Should().Be(1); + } + } + + [Fact] + public void SharedPoolShouldReturnValueFromBucket() + { + var pool = MemoryPool.Shared; + int length; + using (var memory = pool.Rent(4321)) + { + length = memory.Memory.Length; + memory.Memory.Span[0] = 1; + } + + using (var memory = pool.Rent(1024)) + { + memory.Memory.Length.Should().Be(1024); + memory.Memory.Span[0].Should().Be(0); + } + } + + [Fact] + public void CanBorrowMaxLengthArray() + { + var data = new PipeReaderMemoryPool(1024); + using (var rental = data.Rent(2146435071)) + { + rental.Memory.Length.Should().Be(2146435071); + } + } +} diff --git a/Neo4j.Driver/Neo4j.Driver/Internal/Util/PipeReaderMemoryPool.cs b/Neo4j.Driver/Neo4j.Driver/Internal/Util/PipeReaderMemoryPool.cs index 6f9ff5ed4..4bf48a608 100644 --- a/Neo4j.Driver/Neo4j.Driver/Internal/Util/PipeReaderMemoryPool.cs +++ b/Neo4j.Driver/Neo4j.Driver/Internal/Util/PipeReaderMemoryPool.cs @@ -29,7 +29,7 @@ internal sealed class PipeReaderMemoryPool : MemoryPool public PipeReaderMemoryPool(int defaultSize) { _defaultSize = defaultSize; - _pool = ArrayPool.Create(); + _pool = ArrayPool.Create(2146435071, 64); } public override int MaxBufferSize => int.MaxValue; From 53e9009e6d2a5822cedbd79d2bfabd24e9cb45a7 Mon Sep 17 00:00:00 2001 From: Grant Lodge <6323995+thelonelyvulpes@users.noreply.github.com> Date: Wed, 3 Jan 2024 17:50:44 +0000 Subject: [PATCH 2/8] replace some values with established constants and configs --- .../Util/PipeReaderMemoryPoolTests.cs | 13 +++++++++- Neo4j.Driver/Neo4j.Driver/Config.cs | 25 ++++++++++++++----- Neo4j.Driver/Neo4j.Driver/ConfigBuilder.cs | 3 +++ .../Internal/Util/PipeReaderMemoryPool.cs | 5 ++-- 4 files changed, 37 insertions(+), 9 deletions(-) diff --git a/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/PipeReaderMemoryPoolTests.cs b/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/PipeReaderMemoryPoolTests.cs index 88b40fb86..dc51cc0e7 100644 --- a/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/PipeReaderMemoryPoolTests.cs +++ b/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/PipeReaderMemoryPoolTests.cs @@ -36,7 +36,15 @@ public void ShouldRentMemoryInPower(int size, int expectedSize) using var memory = pool.Rent(size); memory.Memory.Length.Should().Be(expectedSize); } - + + [Fact] + public void ShouldRentMemoryInPowerWithDefaultSize() + { + var pool = new PipeReaderMemoryPool(1024); + using var memory = pool.Rent(); + memory.Memory.Length.Should().Be(1024); + } + [Fact] public void ShouldReusePooledObjects() { @@ -96,8 +104,11 @@ public void SharedPoolShouldReturnValueFromBucket() public void CanBorrowMaxLengthArray() { var data = new PipeReaderMemoryPool(1024); + + // 2146435071 is the max length of an array in .NET using (var rental = data.Rent(2146435071)) { + // when returned to the pool, as it exceeds the max size of the pool, it will be discarded rental.Memory.Length.Should().Be(2146435071); } } diff --git a/Neo4j.Driver/Neo4j.Driver/Config.cs b/Neo4j.Driver/Neo4j.Driver/Config.cs index 66be415e1..84891a601 100644 --- a/Neo4j.Driver/Neo4j.Driver/Config.cs +++ b/Neo4j.Driver/Neo4j.Driver/Config.cs @@ -236,7 +236,7 @@ public int MaxIdleConnectionPoolSize /// /// The configuration for the driver's underlying message reading from the network. /// - public MessageReaderConfig MessageReaderConfig { get; internal set; } = new(); + public MessageReaderConfig MessageReaderConfig { get; internal set; } } /// @@ -253,7 +253,7 @@ public sealed class MessageReaderConfig /// host an entire message. User code can provide an implementation for monitoring; by default, the driver will /// allocate a new array pool that does not take advantage of shared memory pools. /// The minimum buffer size to use when renting memory from the pool. The default value - /// is 65,539. + /// is 32,768. /// /// /// @@ -262,20 +262,33 @@ public sealed class MessageReaderConfig /// the , this should only be used when there is complete trust over the usage of /// shared memory buffers in the application as other components may be using the same memory pool. /// - /// If is less than 1. + /// + /// Note using a small value for could cause a degradation in performance. + /// + /// + /// If is less than 1 or greater than 2146435071 + /// public MessageReaderConfig(MemoryPool memoryPool = null, int minBufferSize = -1) { - if (minBufferSize is < -1 or 0) + if (minBufferSize is < -1 or 0 or > 2146435071) { - throw new ArgumentOutOfRangeException(nameof(minBufferSize)); + throw new ArgumentOutOfRangeException(nameof(minBufferSize), minBufferSize, + "Minimum buffer size must be between 1 and 2146435071, leave as -1 to use default."); } DisablePipelinedMessageReader = false; - MinBufferSize = minBufferSize == -1 ? 65_535 + 4 : minBufferSize; + MinBufferSize = minBufferSize == -1 ? Constants.DefaultReadBufferSize : MinBufferSize; MemoryPool = memoryPool ?? new PipeReaderMemoryPool(MinBufferSize); StreamPipeReaderOptions = new(MemoryPool, MinBufferSize, leaveOpen: true); } + internal MessageReaderConfig(Config config) + { + DisablePipelinedMessageReader = false; + MemoryPool = new PipeReaderMemoryPool(config.MaxReadBufferSize); + StreamPipeReaderOptions = new(MemoryPool, config.DefaultReadBufferSize, leaveOpen: true); + } + /// /// As of 5.15, the driver has migrated the underlying message reading mechanism utilizing ; /// this optimizes the reading and memory usage of the driver, and setting this to true will revert the driver to diff --git a/Neo4j.Driver/Neo4j.Driver/ConfigBuilder.cs b/Neo4j.Driver/Neo4j.Driver/ConfigBuilder.cs index 1ce8d4cd3..062d9db1f 100644 --- a/Neo4j.Driver/Neo4j.Driver/ConfigBuilder.cs +++ b/Neo4j.Driver/Neo4j.Driver/ConfigBuilder.cs @@ -37,6 +37,9 @@ internal ConfigBuilder(Config config) /// A instance. internal Config Build() { + // Initialize the message reader config with internal constructor, it can read the default and max read buffer + // sizes. if users have configured a message reader config we will use that instead. + _config.MessageReaderConfig ??= new MessageReaderConfig(_config); return _config; } diff --git a/Neo4j.Driver/Neo4j.Driver/Internal/Util/PipeReaderMemoryPool.cs b/Neo4j.Driver/Neo4j.Driver/Internal/Util/PipeReaderMemoryPool.cs index 4bf48a608..36eff1d26 100644 --- a/Neo4j.Driver/Neo4j.Driver/Internal/Util/PipeReaderMemoryPool.cs +++ b/Neo4j.Driver/Neo4j.Driver/Internal/Util/PipeReaderMemoryPool.cs @@ -15,6 +15,7 @@ using System; using System.Buffers; +using Neo4j.Driver.Internal.IO; namespace Neo4j.Driver.Internal; @@ -25,11 +26,11 @@ internal sealed class PipeReaderMemoryPool : MemoryPool { private readonly int _defaultSize; private readonly ArrayPool _pool; - + public PipeReaderMemoryPool(int defaultSize) { _defaultSize = defaultSize; - _pool = ArrayPool.Create(2146435071, 64); + _pool = ArrayPool.Create(Constants.MaxReadBufferSize, 64); } public override int MaxBufferSize => int.MaxValue; From faa77c57b17c305a892fa0f5f926b7fe386cb111 Mon Sep 17 00:00:00 2001 From: Grant Lodge <6323995+thelonelyvulpes@users.noreply.github.com> Date: Wed, 3 Jan 2024 18:24:12 +0000 Subject: [PATCH 3/8] Add tests for config builder --- .../Internal/Util/ConfigBuildersTests.cs | 31 +++++++++++++++++++ Neo4j.Driver/Neo4j.Driver/Config.cs | 1 + 2 files changed, 32 insertions(+) diff --git a/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/ConfigBuildersTests.cs b/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/ConfigBuildersTests.cs index cd996f283..0638539fe 100644 --- a/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/ConfigBuildersTests.cs +++ b/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/ConfigBuildersTests.cs @@ -13,7 +13,9 @@ // See the License for the specific language governing permissions and // limitations under the License. +using System.Buffers; using FluentAssertions; +using Neo4j.Driver.Internal.IO; using Xunit; namespace Neo4j.Driver.Internal.Util @@ -43,5 +45,34 @@ public void ShouldReturnNewSessionOptions() options2.Database.Should().Be("system"); } } + + [Fact] + public void ShouldConfigureReaderConfig() + { + var cb = new ConfigBuilder(new Config()); + cb.WithDefaultReadBufferSize(128); + var config = cb.Build(); + config.MessageReaderConfig.Should().NotBeNull(); + config.MessageReaderConfig.MinBufferSize.Should().Be(128); + } + + [Fact] + public void ShouldDefaultReaderConfig() + { + var cb = new ConfigBuilder(new Config()); + var config = cb.Build(); + config.MessageReaderConfig.Should().NotBeNull(); + config.MessageReaderConfig.MinBufferSize.Should().Be(Constants.DefaultReadBufferSize); + } + + [Fact] + public void ShouldUseSpecifiedConfigObject() + { + var cb = new ConfigBuilder(new Config()); + cb.WithMessageReaderConfig(new MessageReaderConfig(MemoryPool.Shared)); + var config = cb.Build(); + config.MessageReaderConfig.Should().NotBeNull(); + config.MessageReaderConfig.MemoryPool.Should().Be(MemoryPool.Shared); + } } } diff --git a/Neo4j.Driver/Neo4j.Driver/Config.cs b/Neo4j.Driver/Neo4j.Driver/Config.cs index 84891a601..71390d3a2 100644 --- a/Neo4j.Driver/Neo4j.Driver/Config.cs +++ b/Neo4j.Driver/Neo4j.Driver/Config.cs @@ -285,6 +285,7 @@ public MessageReaderConfig(MemoryPool memoryPool = null, int minBufferSize internal MessageReaderConfig(Config config) { DisablePipelinedMessageReader = false; + MinBufferSize = config.DefaultReadBufferSize; MemoryPool = new PipeReaderMemoryPool(config.MaxReadBufferSize); StreamPipeReaderOptions = new(MemoryPool, config.DefaultReadBufferSize, leaveOpen: true); } From 0a2d531f9859ab3ab61ae224f67eb96ed77651a6 Mon Sep 17 00:00:00 2001 From: Grant Lodge <6323995+thelonelyvulpes@users.noreply.github.com> Date: Wed, 3 Jan 2024 18:38:03 +0000 Subject: [PATCH 4/8] fix unit tests --- Neo4j.Driver/Neo4j.Driver.Tests/TestDriverContext.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Neo4j.Driver/Neo4j.Driver.Tests/TestDriverContext.cs b/Neo4j.Driver/Neo4j.Driver.Tests/TestDriverContext.cs index 8719d1e33..466e49190 100644 --- a/Neo4j.Driver/Neo4j.Driver.Tests/TestDriverContext.cs +++ b/Neo4j.Driver/Neo4j.Driver.Tests/TestDriverContext.cs @@ -23,7 +23,8 @@ internal static class TestDriverContext { static TestDriverContext() { - MockContext = new DriverContext(new Uri("bolt://localhost:7687"), AuthTokenManagers.None, new Config()); + MockContext = new DriverContext(new Uri("bolt://localhost:7687"), AuthTokenManagers.None, + new ConfigBuilder(new Config()).Build()); } public static DriverContext MockContext { get; } From f24bceed5e94c5af3c3406926da5186a8051f1ca Mon Sep 17 00:00:00 2001 From: Grant Lodge <6323995+thelonelyvulpes@users.noreply.github.com> Date: Thu, 4 Jan 2024 17:47:20 +0000 Subject: [PATCH 5/8] Add a parameter, ensure we use the correct max value for the pool --- .../Util/PipeReaderMemoryPoolTests.cs | 9 ++--- .../Internal/Util/PipeReaderMemoryPool.cs | 6 ++-- Neo4j.Driver/Neo4j.Driver/Public/Config.cs | 35 ++++++++++++++----- 3 files changed, 35 insertions(+), 15 deletions(-) diff --git a/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/PipeReaderMemoryPoolTests.cs b/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/PipeReaderMemoryPoolTests.cs index dc51cc0e7..16141a71a 100644 --- a/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/PipeReaderMemoryPoolTests.cs +++ b/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/PipeReaderMemoryPoolTests.cs @@ -15,6 +15,7 @@ using System.Buffers; using FluentAssertions; +using Neo4j.Driver.Internal.IO; using Xunit; namespace Neo4j.Driver.Internal.Util; @@ -32,7 +33,7 @@ public class PipeReaderMemoryPoolTests [InlineData(65539, 131072)] public void ShouldRentMemoryInPower(int size, int expectedSize) { - var pool = new PipeReaderMemoryPool(1024); + var pool = new PipeReaderMemoryPool(1024, Constants.MaxReadBufferSize); using var memory = pool.Rent(size); memory.Memory.Length.Should().Be(expectedSize); } @@ -40,7 +41,7 @@ public void ShouldRentMemoryInPower(int size, int expectedSize) [Fact] public void ShouldRentMemoryInPowerWithDefaultSize() { - var pool = new PipeReaderMemoryPool(1024); + var pool = new PipeReaderMemoryPool(1024, Constants.MaxReadBufferSize); using var memory = pool.Rent(); memory.Memory.Length.Should().Be(1024); } @@ -48,7 +49,7 @@ public void ShouldRentMemoryInPowerWithDefaultSize() [Fact] public void ShouldReusePooledObjects() { - var pool = new PipeReaderMemoryPool(1024); + var pool = new PipeReaderMemoryPool(1024, Constants.MaxReadBufferSize); int length; using (var memory = pool.Rent(4321)) @@ -103,7 +104,7 @@ public void SharedPoolShouldReturnValueFromBucket() [Fact] public void CanBorrowMaxLengthArray() { - var data = new PipeReaderMemoryPool(1024); + var data = new PipeReaderMemoryPool(1024, Constants.MaxReadBufferSize); // 2146435071 is the max length of an array in .NET using (var rental = data.Rent(2146435071)) diff --git a/Neo4j.Driver/Neo4j.Driver/Internal/Util/PipeReaderMemoryPool.cs b/Neo4j.Driver/Neo4j.Driver/Internal/Util/PipeReaderMemoryPool.cs index da7c0c15a..91aaf7365 100644 --- a/Neo4j.Driver/Neo4j.Driver/Internal/Util/PipeReaderMemoryPool.cs +++ b/Neo4j.Driver/Neo4j.Driver/Internal/Util/PipeReaderMemoryPool.cs @@ -27,10 +27,10 @@ internal sealed class PipeReaderMemoryPool : MemoryPool private readonly int _defaultSize; private readonly ArrayPool _pool; - public PipeReaderMemoryPool(int defaultSize) + public PipeReaderMemoryPool(int defaultBufferSize, int maxPooledBufferSize) { - _defaultSize = defaultSize; - _pool = ArrayPool.Create(Constants.MaxReadBufferSize, 64); + _defaultSize = defaultBufferSize; + _pool = ArrayPool.Create(maxPooledBufferSize, 64); } public override int MaxBufferSize => int.MaxValue; diff --git a/Neo4j.Driver/Neo4j.Driver/Public/Config.cs b/Neo4j.Driver/Neo4j.Driver/Public/Config.cs index 67c8d339e..126d4d1e3 100644 --- a/Neo4j.Driver/Neo4j.Driver/Public/Config.cs +++ b/Neo4j.Driver/Neo4j.Driver/Public/Config.cs @@ -247,14 +247,17 @@ public sealed class MessageReaderConfig { /// /// Constructs a new instance of .
- /// The configuration for the driver's underlying message reading from the network. + /// The configuration for the driver's underlying message reading from the network.
+ /// Using this constructor overrides the and . ///
/// The memory pool for creating buffers when reading messages. The PipeReader will borrow /// memory from the pool of at least ReadBufferSize size. The message reader can request larger memory blocks to /// host an entire message. User code can provide an implementation for monitoring; by default, the driver will /// allocate a new array pool that does not take advantage of shared memory pools. - /// The minimum buffer size to use when renting memory from the pool. The default value - /// is 32,768. + /// The minimum buffer size to use when renting memory from the pool. + ///
The default value is 32,768. + /// The maximum buffer size to use when renting memory from neo4j's default pool. + ///
The default is 131,072. /// /// /// @@ -264,22 +267,38 @@ public sealed class MessageReaderConfig /// shared memory buffers in the application as other components may be using the same memory pool. /// /// + /// The will define it's own maximum pooled buffer size, but must be able to provide + /// an memory object upto the limit 2146435071 bytes. The will not be observed + /// when the is passed.. + /// + /// /// Note using a small value for could cause a degradation in performance. /// /// /// If is less than 1 or greater than 2146435071 /// - public MessageReaderConfig(MemoryPool memoryPool = null, int minBufferSize = -1) + /// + /// If is less than or greater than 2146435071 + /// + public MessageReaderConfig(MemoryPool memoryPool = null, int minBufferSize = -1, int maxPooledBufferSize = -1) { if (minBufferSize is < -1 or 0 or > 2146435071) { throw new ArgumentOutOfRangeException(nameof(minBufferSize), minBufferSize, "Minimum buffer size must be between 1 and 2146435071, leave as -1 to use default."); } - - DisablePipelinedMessageReader = false; MinBufferSize = minBufferSize == -1 ? Constants.DefaultReadBufferSize : MinBufferSize; - MemoryPool = memoryPool ?? new PipeReaderMemoryPool(MinBufferSize); + if (maxPooledBufferSize != -1 && maxPooledBufferSize < MinBufferSize || maxPooledBufferSize> 2146435071) + { + throw new ArgumentOutOfRangeException( + nameof(maxPooledBufferSize), + maxPooledBufferSize, + $"Max pooled buffer size buffer size must be greater than minBufferSize({MinBufferSize}), leave as -1 to use default."); + } + + DisablePipelinedMessageReader = false; + MemoryPool = memoryPool ?? new PipeReaderMemoryPool(MinBufferSize, + maxPooledBufferSize == -1 ? Constants.MaxReadBufferSize : maxPooledBufferSize); StreamPipeReaderOptions = new(MemoryPool, MinBufferSize, leaveOpen: true); } @@ -287,7 +306,7 @@ internal MessageReaderConfig(Config config) { DisablePipelinedMessageReader = false; MinBufferSize = config.DefaultReadBufferSize; - MemoryPool = new PipeReaderMemoryPool(config.MaxReadBufferSize); + MemoryPool = new PipeReaderMemoryPool(config.DefaultReadBufferSize, config.MaxReadBufferSize); StreamPipeReaderOptions = new(MemoryPool, config.DefaultReadBufferSize, leaveOpen: true); } From 0619990c4e69b32de0ee085759cb020a730633c6 Mon Sep 17 00:00:00 2001 From: Grant Lodge <6323995+thelonelyvulpes@users.noreply.github.com> Date: Fri, 5 Jan 2024 09:54:23 +0000 Subject: [PATCH 6/8] fix test --- .../Internal/Util/PipeReaderMemoryPoolTests.cs | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/PipeReaderMemoryPoolTests.cs b/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/PipeReaderMemoryPoolTests.cs index 16141a71a..052b5cef9 100644 --- a/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/PipeReaderMemoryPoolTests.cs +++ b/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/PipeReaderMemoryPoolTests.cs @@ -65,18 +65,22 @@ public void ShouldReusePooledObjects() } } + /// + /// This test is to verify the behaviour of the shared pool. + /// It is an unnecessary test but it proves the behaviour to validate + /// [Fact] public void SharedPoolShouldReturnSameValue() { var pool = MemoryPool.Shared; int length; - using (var memory = pool.Rent(4321)) + using (var memory = pool.Rent(1024)) { length = memory.Memory.Length; memory.Memory.Span[0] = 1; } - using (var memory = pool.Rent(4321)) + using (var memory = pool.Rent(1024)) { memory.Memory.Length.Should().Be(length); memory.Memory.Span[0].Should().Be(1); @@ -84,16 +88,16 @@ public void SharedPoolShouldReturnSameValue() } [Fact] - public void SharedPoolShouldReturnValueFromBucket() + public void ShouldNotReturnSharedPoolObjects() { var pool = MemoryPool.Shared; - int length; - using (var memory = pool.Rent(4321)) + using (var memory = pool.Rent(1024)) { - length = memory.Memory.Length; + memory.Memory.Length.Should().Be(1024); memory.Memory.Span[0] = 1; } - + + pool = new PipeReaderMemoryPool(1024, 2048); using (var memory = pool.Rent(1024)) { memory.Memory.Length.Should().Be(1024); From 2fe152ef8cadab886b9293b6c8f13c83520c4fe1 Mon Sep 17 00:00:00 2001 From: Grant Lodge <6323995+thelonelyvulpes@users.noreply.github.com> Date: Fri, 5 Jan 2024 09:58:56 +0000 Subject: [PATCH 7/8] add test case --- .../Util/PipeReaderMemoryPoolTests.cs | 89 +++++++++++-------- 1 file changed, 54 insertions(+), 35 deletions(-) diff --git a/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/PipeReaderMemoryPoolTests.cs b/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/PipeReaderMemoryPoolTests.cs index 052b5cef9..3069466b3 100644 --- a/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/PipeReaderMemoryPoolTests.cs +++ b/Neo4j.Driver/Neo4j.Driver.Tests/Internal/Util/PipeReaderMemoryPoolTests.cs @@ -23,27 +23,28 @@ namespace Neo4j.Driver.Internal.Util; public class PipeReaderMemoryPoolTests { [Theory] - [InlineData(512, 512)] - [InlineData(1024, 1024)] - [InlineData(1025, 2048)] - [InlineData(2049, 4096)] - [InlineData(4096, 4096)] - [InlineData(4097, 8192)] - [InlineData(65535, 65536)] - [InlineData(65539, 131072)] - public void ShouldRentMemoryInPower(int size, int expectedSize) + [InlineData(512, 512, Constants.MaxReadBufferSize)] + [InlineData(1024, 1024, Constants.MaxReadBufferSize)] + [InlineData(1025, 2048, Constants.MaxReadBufferSize)] + [InlineData(2049, 4096, Constants.MaxReadBufferSize)] + [InlineData(4096, 4096, Constants.MaxReadBufferSize)] + [InlineData(4097, 8192, Constants.MaxReadBufferSize)] + [InlineData(65535, 65536, Constants.MaxReadBufferSize)] + [InlineData(65539, 131072, Constants.MaxReadBufferSize)] + [InlineData(1025, 1025, 1024)] + public void ShouldRentMemoryInPower(int size, int expectedSize, int maxReadBufferSize) { - var pool = new PipeReaderMemoryPool(1024, Constants.MaxReadBufferSize); - using var memory = pool.Rent(size); - memory.Memory.Length.Should().Be(expectedSize); + var pool = new PipeReaderMemoryPool(1024, maxReadBufferSize); + using var memoryOwner = pool.Rent(size); + memoryOwner.Memory.Length.Should().Be(expectedSize); } [Fact] public void ShouldRentMemoryInPowerWithDefaultSize() { var pool = new PipeReaderMemoryPool(1024, Constants.MaxReadBufferSize); - using var memory = pool.Rent(); - memory.Memory.Length.Should().Be(1024); + using var memoryOwner = pool.Rent(); + memoryOwner.Memory.Length.Should().Be(1024); } [Fact] @@ -52,16 +53,16 @@ public void ShouldReusePooledObjects() var pool = new PipeReaderMemoryPool(1024, Constants.MaxReadBufferSize); int length; - using (var memory = pool.Rent(4321)) + using (var memoryOwner = pool.Rent(4321)) { - length = memory.Memory.Length; - memory.Memory.Span[0] = 1; + length = memoryOwner.Memory.Length; + memoryOwner.Memory.Span[0] = 1; } - using (var memory = pool.Rent(4321)) + using (var memoryOwner = pool.Rent(4321)) { - memory.Memory.Length.Should().Be(length); - memory.Memory.Span[0].Should().Be(1); + memoryOwner.Memory.Length.Should().Be(length); + memoryOwner.Memory.Span[0].Should().Be(1); } } @@ -74,16 +75,16 @@ public void SharedPoolShouldReturnSameValue() { var pool = MemoryPool.Shared; int length; - using (var memory = pool.Rent(1024)) + using (var memoryOwner = pool.Rent(1024)) { - length = memory.Memory.Length; - memory.Memory.Span[0] = 1; + length = memoryOwner.Memory.Length; + memoryOwner.Memory.Span[0] = 1; } - using (var memory = pool.Rent(1024)) + using (var memoryOwner = pool.Rent(1024)) { - memory.Memory.Length.Should().Be(length); - memory.Memory.Span[0].Should().Be(1); + memoryOwner.Memory.Length.Should().Be(length); + memoryOwner.Memory.Span[0].Should().Be(1); } } @@ -91,30 +92,48 @@ public void SharedPoolShouldReturnSameValue() public void ShouldNotReturnSharedPoolObjects() { var pool = MemoryPool.Shared; - using (var memory = pool.Rent(1024)) + using (var memoryOwner = pool.Rent(1024)) { - memory.Memory.Length.Should().Be(1024); - memory.Memory.Span[0] = 1; + memoryOwner.Memory.Length.Should().Be(1024); + memoryOwner.Memory.Span[0] = 1; } pool = new PipeReaderMemoryPool(1024, 2048); - using (var memory = pool.Rent(1024)) + using (var memoryOwner = pool.Rent(1024)) { - memory.Memory.Length.Should().Be(1024); - memory.Memory.Span[0].Should().Be(0); + memoryOwner.Memory.Length.Should().Be(1024); + memoryOwner.Memory.Span[0].Should().Be(0); } } [Fact] public void CanBorrowMaxLengthArray() { - var data = new PipeReaderMemoryPool(1024, Constants.MaxReadBufferSize); + var pool = new PipeReaderMemoryPool(1024, Constants.MaxReadBufferSize); // 2146435071 is the max length of an array in .NET - using (var rental = data.Rent(2146435071)) + using (var memoryOwner = pool.Rent(2146435071)) { // when returned to the pool, as it exceeds the max size of the pool, it will be discarded - rental.Memory.Length.Should().Be(2146435071); + memoryOwner.Memory.Length.Should().Be(2146435071); + } + } + + [Fact] + public void ShouldNotStoreLargerThanMaxReadBufferSize() + { + var pool = new PipeReaderMemoryPool(1024, 1024); + + using (var memory = pool.Rent(1025)) + { + memory.Memory.Length.Should().Be(1025); + memory.Memory.Span[0] = 1; + } + + using (var memory = pool.Rent(1025)) + { + memory.Memory.Length.Should().Be(1025); + memory.Memory.Span[0].Should().Be(0); } } } From a611d495c870a738d39c2c87aeba51c550e92c2c Mon Sep 17 00:00:00 2001 From: Grant Lodge <6323995+thelonelyvulpes@users.noreply.github.com> Date: Fri, 5 Jan 2024 10:35:45 +0000 Subject: [PATCH 8/8] tidy --- Neo4j.Driver/Neo4j.Driver/Public/Config.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Neo4j.Driver/Neo4j.Driver/Public/Config.cs b/Neo4j.Driver/Neo4j.Driver/Public/Config.cs index 126d4d1e3..e6df52818 100644 --- a/Neo4j.Driver/Neo4j.Driver/Public/Config.cs +++ b/Neo4j.Driver/Neo4j.Driver/Public/Config.cs @@ -282,13 +282,14 @@ public sealed class MessageReaderConfig /// public MessageReaderConfig(MemoryPool memoryPool = null, int minBufferSize = -1, int maxPooledBufferSize = -1) { - if (minBufferSize is < -1 or 0 or > 2146435071) + const int maxArrayLength = 2146435071; + if (minBufferSize is < -1 or 0 or > maxArrayLength) { throw new ArgumentOutOfRangeException(nameof(minBufferSize), minBufferSize, "Minimum buffer size must be between 1 and 2146435071, leave as -1 to use default."); } MinBufferSize = minBufferSize == -1 ? Constants.DefaultReadBufferSize : MinBufferSize; - if (maxPooledBufferSize != -1 && maxPooledBufferSize < MinBufferSize || maxPooledBufferSize> 2146435071) + if (maxPooledBufferSize != -1 && (maxPooledBufferSize < MinBufferSize || maxPooledBufferSize > maxArrayLength)) { throw new ArgumentOutOfRangeException( nameof(maxPooledBufferSize),