-
Notifications
You must be signed in to change notification settings - Fork 23
Return ArrayPool buffer to pool on success only, skip return on exception #66
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -227,16 +227,10 @@ await stream | |
| public async Task ReadAsync(IDataItem[] dataItems, CancellationToken cancellationToken = default) | ||
| { | ||
| var results = ArrayPool<ReadWriteErrorCode>.Shared.Rent(dataItems.Length); | ||
| try | ||
| { | ||
| await ReadAsync(dataItems, results, cancellationToken).ConfigureAwait(false); | ||
| await ReadAsync(dataItems, results, cancellationToken).ConfigureAwait(false); | ||
|
|
||
| ReadWriteErrorHelpers.ThrowIfHasErrors("Read", dataItems, results.AsSpan(0, dataItems.Length)); | ||
| } | ||
| finally | ||
| { | ||
| ArrayPool<ReadWriteErrorCode>.Shared.Return(results); | ||
| } | ||
| ReadWriteErrorHelpers.ThrowIfHasErrors("Read", dataItems, results.AsSpan(0, dataItems.Length)); | ||
| ArrayPool<ReadWriteErrorCode>.Shared.Return(results); | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -297,16 +291,10 @@ public async Task ReadAsync(IDataItem[] dataItems, Memory<ReadWriteErrorCode> re | |
| public async Task WriteAsync(IDataItem[] dataItems, CancellationToken cancellationToken = default) | ||
| { | ||
| var results = ArrayPool<ReadWriteErrorCode>.Shared.Rent(dataItems.Length); | ||
| try | ||
| { | ||
| await WriteAsync(dataItems, results, cancellationToken).ConfigureAwait(false); | ||
| await WriteAsync(dataItems, results, cancellationToken).ConfigureAwait(false); | ||
|
|
||
| ReadWriteErrorHelpers.ThrowIfHasErrors("Write", dataItems, results.AsSpan(0, dataItems.Length)); | ||
| } | ||
| finally | ||
| { | ||
| ArrayPool<ReadWriteErrorCode>.Shared.Return(results); | ||
| } | ||
| ReadWriteErrorHelpers.ThrowIfHasErrors("Write", dataItems, results.AsSpan(0, dataItems.Length)); | ||
| ArrayPool<ReadWriteErrorCode>.Shared.Return(results); | ||
|
Comment on lines
293
to
+297
|
||
| } | ||
|
|
||
| /// <summary> | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ArrayPool<ReadWriteErrorCode>.Shared.Rent(...)is no longer paired with aReturn(...)on exception paths (including the common path whereThrowIfHasErrorsthrows for per-item failures). This defeats pooling under failure conditions and can cause sustained allocations/GC pressure (and potentially LOH allocations for largedataItems.Length) when a PLC is offline or returns item errors repeatedly.Consider restoring a
try/finallythat always returns the buffer, or if the intent is specifically to avoid returning on transport/cancellation exceptions, then catch only the expectedAggregateExceptionfromThrowIfHasErrors(return the buffer, then rethrow) while still letting other exceptions skip return if you really need that behavior.