Repository navigation
NCBC-4383: Dispose the builder pools in the MutateIn boundary test - #240
Merged
Merged
Conversation
Motivation ========== SendAsync_FirstSpecEndsInLastEightBytesOfBuffer_DoesNotThrow creates a DefaultObjectPool on each of its 200 iterations. That pool never disposes its builders, so their ArrayPool<byte>.Shared arrays were never returned. On net48 the pool counts rented slots and serves the next larger bucket once a bucket is fully rented, so the 32 KiB and 64 KiB buckets stayed full. Tests that ran later and rented 64 KiB got 131072 bytes, and OperationBuilderTests failed on Windows net48, depending on test order. Modification ============ Create each iteration's pool with DefaultObjectPoolProvider and dispose it after the send. The pool stays per iteration: a shared one would keep the grown buffer, and the test would no longer reach the boundary it checks. Results ======= The test returns every buffer it rents and still fails without the NCBC-4376 fix.
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues were identified for the test-only change to dispose each iteration’s builder pool.
No open issues 🎉
What changed in this PR
Fixes buffer-pool cleanup in the MutateIn boundary regression test, preventing test-order-dependent buffer-size failures on .NET Framework without changing production code.
Changes:
- Creates each iteration’s builder pool through
DefaultObjectPoolProvider. - Disposes the pool in
finally, returning pooled buffers while preserving a fresh pool for each boundary check.
| File | Description |
|---|---|
| tests/Couchbase.UnitTests/Core/IO/Operations/SubDocument/MultiMutationTests.cs | Disposes each iteration’s builder pool to prevent shared-buffer leaks. |
Review effort: Balanced
emilienbev
reviewed
Oct 30, 2026
emilienbev
left a comment
emilienbev
left a comment
Contributor
There was a problem hiding this comment.
Thanks, this looks good to me. Could the lower-bound asserts in OperationBuilderTests from #239 move here? They fix the same flaky tests.
Contributor
Author
|
They could. In fact should really. I'll do that. |
Motivation ========== The EnsureCapacity growth tests asserted the exact capacity, which is the size of the array that ArrayPool<byte>.Shared returns, not a value the builder chooses. On net48 the pool serves the next larger bucket once all slots of the exact bucket are rented, so any test that holds 64 KiB buffers can make the *_Quadruples tests see 131072. Modification ============ Assert that the capacity is at least the requested size. Rename the four tests from *_Doubles and *_Quadruples to *_Grows. Results ======= The tests no longer depend on what other tests rent from the shared pool.
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The issue in brief
A unit test added by NCBC-4376 borrows buffers from a shared pool and never gives them back. On .NET Framework, the shared pool keeps count of buffers that are out. After this test it thinks the 32 KiB and 64 KiB sizes are all in use, so later requests for 64 KiB get a 128 KiB buffer.
Some later tests check the exact buffer size, so they then fail on the Windows net48 legs. Whether they fail depends on which test classes run first. This made
OperationBuilderTestsfail on master (run 40765292781) and three times in a row on PR #239. Production code is not affected: the SDK's own builder pool disposes builders it drops.Change
SendAsync_FirstSpecEndsInLastEightBytesOfBuffer_DoesNotThrowstill makes a new pool for each of its 200 iterations, so every send starts from a buffer that has not grown. The pool now comes fromDefaultObjectPoolProvider, which disposes the builders it holds, and the test disposes it after each send. One shared pool would not work:Reset()keeps the grown capacity, so the test would stop reaching the buffer boundary it checks.The
EnsureCapacitygrowth tests inOperationBuilderTests(moved here from #239, as asked in review) now assert that the capacity is at least the requested size, not an exact size. The exact size came from the shared pool, not from the builder, so a test that holds pool buffers could still change it. The four tests are renamed from*_Doublesand*_Quadruplesto*_Grows.Tests
ArrayPool<byte>.Sharedwith a probe and ran the net48 tests on Mono. Before the change, this test left 50 64 KiB buffers out for the rest of the process. After it, none.OperationBuilder, the test still fails.MultiMutationTestspass on net8.0, and on net48 under Mono.MultiMutationTestsandOperationBuilderTestspass on net10.0.Jira: NCBC-4383
🤖 Generated with Claude Code