IoUring: Stop generic FileRegion drain loop when transferred() reache…#16832
Merged
Conversation
…s count() (#16826) Closes #16825. The generic `FileRegion` fallback added in #16571 drains the region only on `buf.writableBytes() > 0`: ```java while (buf.writableBytes() > 0) { long t = region.transferTo(ch, region.transferred()); if (t <= 0) { break; } } ``` epoll's equivalent at `AbstractEpollStreamChannel#writeFileRegion` guards the call with: ```java if (region.transferred() >= region.count()) { in.remove(); return 0; } ``` so epoll never invokes `transferTo()` past `transferred() == count()`. The io_uring fallback can, which violates the `FileRegion` contract for implementations that lazily emit per-chunk framing — the extra call builds an empty next chunk and ships its framing into the buffer, breaking the receiver. See #16825 for the full diagnosis. Bound the inner drain loop with `region.transferred() < region.count()` in addition to `buf.writableBytes() > 0`, mirroring epoll's short-circuit. Add `IoUringSocketFileRegionTest#testFileRegionDrainStopsAtCompletion` plus an `OvershootDetectingFileRegion` fixture that records every `transferTo()` invocation made past `transferred() == count()`. The fixture writes one byte and advances `transferred` to `count` in one shot, exercising the buggy path (15 overshoots on default chunk size) and the fixed path (0 overshoots) deterministically. A few test-design notes for review: - Skips on non-io_uring client permutations — the drain loop is on the sending side. - Skips when `io.netty.iouring.fileRegionChunkSize` is set to 1, since that leaves no spare capacity for the overshoot to land in. - Asserts on the writer-side counter synchronously after `writeAndFlush(...).sync()`; no time-based wait. - Routes contract violations through `IOException` rather than `AssertionError` so they surface on the write future's cause through the transport's `catch (Exception)` instead of wedging the EventLoop. - `@Timeout(value = 30, unit = SECONDS)` as a safety net for any future regression that wedges `sync()`. io_uring's generic `FileRegion` path stops over-calling `transferTo()`. Custom `FileRegion` implementations that lazily emit per-chunk framing work under io_uring with the same semantics they already have under NIO and epoll.
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.
…s count() (#16826)
Closes #16825.
The generic
FileRegionfallback added in #16571 drains the region only onbuf.writableBytes() > 0:epoll's equivalent at
AbstractEpollStreamChannel#writeFileRegionguards the call with:so epoll never invokes
transferTo()pasttransferred() == count(). The io_uring fallback can, which violates theFileRegioncontract for implementations that lazily emit per-chunk framing — the extra call builds an empty next chunk and ships its framing into the buffer, breaking the receiver. See #16825 for the full diagnosis.Bound the inner drain loop with
region.transferred() < region.count()in addition tobuf.writableBytes() > 0, mirroring epoll's short-circuit.Add
IoUringSocketFileRegionTest#testFileRegionDrainStopsAtCompletionplus anOvershootDetectingFileRegionfixture that records everytransferTo()invocation made pasttransferred() == count(). The fixture writes one byte and advancestransferredtocountin one shot, exercising the buggy path (15 overshoots on default chunk size) and the fixed path (0 overshoots) deterministically.A few test-design notes for review:
io.netty.iouring.fileRegionChunkSizeis set to 1, since that leaves no spare capacity for the overshoot to land in.writeAndFlush(...).sync(); no time-based wait.IOExceptionrather thanAssertionErrorso they surface on the write future's cause through the transport'scatch (Exception)instead of wedging the EventLoop.@Timeout(value = 30, unit = SECONDS)as a safety net for any future regression that wedgessync().io_uring's generic
FileRegionpath stops over-callingtransferTo(). CustomFileRegionimplementations that lazily emit per-chunk framing work under io_uring with the same semantics they already have under NIO and epoll.