Return ArrayPool buffer to pool on success only, skip return on exception

[Copilot]

ArrayPool buffers rented in ReadAsync/WriteAsync were previously returned unconditionally via finally. The intent is to skip the return on exception paths (letting the GC handle it) while still returning on success.

Changes

• Removed try/finally blocks in ReadAsync(IDataItem[], CancellationToken) and WriteAsync(IDataItem[], CancellationToken)
• ArrayPool<ReadWriteErrorCode>.Shared.Return(results) is now placed after ThrowIfHasErrors — only reached when no exception is thrown

var results = ArrayPool<ReadWriteErrorCode>.Shared.Rent(dataItems.Length);
await ReadAsync(dataItems, results, cancellationToken).ConfigureAwait(false);

ReadWriteErrorHelpers.ThrowIfHasErrors("Read", dataItems, results.AsSpan(0, dataItems.Length));
ArrayPool<ReadWriteErrorCode>.Shared.Return(results); // only on success

[mycroes]

@gfoidl Actually had CoPilot do this for me, so didn't need a computer to commit it 😆

[Copilot]

Pull request overview

This PR changes how ArrayPool<ReadWriteErrorCode> buffers are managed in S7Connection.ReadAsync(IDataItem[], CancellationToken) and WriteAsync(IDataItem[], CancellationToken), returning the rented buffer only when the operation completes without throwing.

Changes:

• Removed try/finally blocks around pooled results buffers in ReadAsync(...) and WriteAsync(...).
• Moved ArrayPool<ReadWriteErrorCode>.Shared.Return(results) to occur only after ReadWriteErrorHelpers.ThrowIfHasErrors(...) completes successfully.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

ArrayPool<ReadWriteErrorCode>.Shared.Rent(...) is no longer paired with a Return(...) on exception paths (including the common path where ThrowIfHasErrors throws for per-item failures). This defeats pooling under failure conditions and can cause sustained allocations/GC pressure (and potentially LOH allocations for large dataItems.Length) when a PLC is offline or returns item errors repeatedly.

Consider restoring a try/finally that always returns the buffer, or if the intent is specifically to avoid returning on transport/cancellation exceptions, then catch only the expected AggregateException from ThrowIfHasErrors (return the buffer, then rethrow) while still letting other exceptions skip return if you really need that behavior.

[Copilot]

Same pooling issue as in ReadAsync: if WriteAsync(...) throws (including ThrowIfHasErrors throwing AggregateException for per-item failures), the rented ArrayPool buffer is never returned. This can significantly increase allocation rate and memory pressure in repeated error scenarios.

Recommendation: ensure the rented buffer is returned for at least the expected error case (AggregateException from ThrowIfHasErrors) and ideally via try/finally to keep pooling effective and consistent with the existing ArrayPool<byte> usage earlier in this file.