[He-Pin]

Motivation

TLS GraphStage fails when the TCP buffer pool is smaller than the SSLEngine packet requirement. The fixed-size pool also cannot satisfy output growth. CI additionally exposed a race in the partial-record regression: the test sampled session-query counts before the stage processed the completed handshake.

Modification

• Fall back to stage-owned heap buffers, sharing one initial allocation between three disjoint transport slices. Keep heap and incorrectly sized buffers out of the TCP pool.
• Flush pending ciphertext before growing, preserve data during resizing, refresh session requirements and batching, and shrink drained output after renegotiation permits it.
• Cover TLS 1.2/1.3 with small buffers, growth, heap sharing, pool ownership and cancellation draining; document the allocation behavior.
• Make the test harness await session verification before sampling query counts. Assert the final count after both input cancellations, when unwrap post-processing has completed.

Result

TLS progresses with small TCP buffers while preserving ciphertext and pool ownership. The CI regression now establishes the required ordering and still detects unnecessary session queries.

Tests

• JDK 17: 312 related TLS/TCP tests passed, including 28 focused buffer and close cases.
• Negative control: temporarily querying session sizes on every underflow makes the regression fail with 3 was not equal to 2; the mutation was restored.
• Native scalafmt, headerCreateAll, checkCodeStyle (including Java formatting and cross-version headers), and git diff --check passed.
• +mimaReportBinaryIssues passed on Scala 2.13.18 and 3.3.9.
• sortImports encountered a Scalafix/Scalameta NoSuchMethodError; unrelated import edits were reverted and the native formatter check passed.
• At the user's request, local docs/paradox was stopped and validatePullRequest was not run. The remaining documentation and Scala test matrix validation is delegated to remote CI.

References

Refs #2860, Refs #3727, Refs #3728.

[pjfanning]

Thanks for this. Notes from a review pass. The inline comments cover changed lines; the points below are about unchanged code whose behaviour this PR changes. Up to now, a buffer larger than direct-buffer-size threw an exception. With this PR it falls back to the heap, so these paths can now be reached.

1. Overflow grows the buffer instead of flushing (doWrap, growTransportOutBuffer). On BUFFER_OVERFLOW the stage always doubles transportOutBuffer, even when the overflow happened only because deferred ciphertext is still in it (up to DeferredTransportFlushLimit = 16 deferred flushes). Sending the pending bytes would free the space. Each overflow inside that window doubles the buffer again, so it can grow geometrically for no reason. Suggestion: if transportOutBuffer.position() > 0, call flushToTransport() and retry the wrap, and grow only when the buffer is already empty.

2. A grown buffer is never shrunk (growTransportOutBuffer). After one transient overflow, the pooled direct buffer goes back to the pool and the connection holds a heap buffer of 2x that size until it closes. Every later wrap then copies between heap and native memory. On long-lived connections that doubles the memory used for transport output. Consider returning to a pooled-size buffer once the large one has been drained.

3. Inbound buffers have no headroom and never grow (allocateBuffers, doUnwrap, putBackUnreadBuffer). In the small-pool fallback, transportInBuffer and transportUnreadBuffer are heap buffers sized exactly to the getPacketBufferSize() read before the handshake. Nothing grows them afterwards: BUFFER_UNDERFLOW and BUFFER_OVERFLOW only flush or put data back. If the negotiated session needs larger packets (for example with jsse.SSLEngine.acceptLargeFragments), a full record can never fit, and the connection stalls or putBackUnreadBuffer throws. The old 128 KiB pooled buffers hid this.

4. Batching still depends on the pool size (applicationBufferSize). recordsThatFitTransportBuffer is still pooledBufferSize / packetSize. With an 8 KiB pool that is always 1 record per engine call, even after transportOutBuffer has grown, so throughput depends on an unrelated TCP setting. Deriving it from the transport buffer's actual capacity, or from the packet size, would remove that dependency.

5. Heap allocation on every connection (allocateBuffers). With a small pool, each materialization allocates 3 heap buffers of about 16.7 KiB (~50 KiB per handshake), with no reuse. That adds GC pressure on servers with many short TLS connections. A TLS-specific pool sized to the packet size, or one allocation sliced into the three buffers, would help.

[He-Pin]

Addressed in 52b44b1: flush before growth, resize inbound buffers for changed session requirements, and derive batching from actual capacity. Growth now occurs only when an empty buffer is too small, so shrinking would repeat the required allocation. A TLS-specific pool remains separate performance work; the fallback cost is documented.

The inbound-growth tests now use fixed initial session sizes and assert overflow/underflow occurred. The 256 TLS tests and 9 focused cases on JDK 17/21/25 pass; all four inline threads are resolved.

[He-Pin]

Deep review, comparing against TLSActor.

The premise checks out. TLSActor uses fixed heap buffers (16665 + 2048, TLSActor.scala:151-159) and never touches the TCP pool, so none of these failure modes can occur there. For the GraphStage engine:

• The default pekko.io.tcp.direct-buffer-size is 128 KiB (actor/src/main/resources/reference.conf:1018) and the pool is new DirectByteBufferPool(Settings.DirectBufferSize, Settings.MaxDirectBufferPoolSize) (Tcp.scala:667). Measured on JDK 25: getPacketBufferSize() = 16709, getApplicationBufferSize() = 16704, identical before and after beginHandshake() for both TLSv1.2 and TLSv1.3. So with default settings the old acquireTransportBuffer never throws - the initialization failure needs a lowered direct-buffer-size, which is exactly what the new TlsGraphStageSmallBufferSpec exercises.
• Flush-before-grow is the right order: the overflow is normally caused by ciphertext already sitting in transportOutBuffer, and growing there would double the buffer for no reason. flush deferred cipher bytes before retrying an overflow without growing the buffer pins that down nicely, including the largestOutputCapacity shouldBe poolBufferSize assertion.
• The pool-ownership assertion (acquire the whole free list, then check isDirect, capacity and pairwise identity) is a genuinely good test: DirectByteBufferPool.release performs no validation, so a double release or a foreign buffer would otherwise go unnoticed until some unrelated TCP connection misbehaves.

Findings: four suggestions and three nits inline, no blockers. The buffer mechanics themselves all check out - I traced acquire/release ownership, ensureReadBufferCapacity preserving read-mode leftovers, the growth ordering relative to putBackUnreadBuffer, and growUserOutBuffer being called only after flushToUser() has cleared userOutBuffer, and could not find a leak, a stale-reference use, or a BufferOverflowException window in expanded.put(buffer). I also confirmed the new BUFFER_OVERFLOW flush cannot trip the "TLS output slot already has pending data" guard, since every doWrap call site is gated on transportOutput.isDemandAvailable, which requires pending == null.

Conflict with #3727 - the two branches are independent, both based on a55a008, and both edit the same lines of stream-io.md, TlsGraphStage.scala and TlsGraphStageIsolatedSpec.scala. The spec conflict is the awkward one: both change ScriptedSSLEngine's constructor and newHarness's signature incompatibly, and both append cases to the same "TlsGraphStage deferred close handling" must { block. Worth agreeing an order and rebasing the second.

Note that Check / Tests (scala3) was still pending when I looked; everything else was green.

[He-Pin]

BUFFER_UNDERFLOW is the ordinary "the record is not complete yet" path, so this now runs on every partial TLS record - engine.getSession() plus two accessors plus two capacity comparisons - and the same again in the BUFFER_OVERFLOW branch at line 480.

Measured on JDK 25, getPacketBufferSize()/getApplicationBufferSize() return 16709/16704 both before and after beginHandshake(), for TLSv1.2 and TLSv1.3 alike, so with the default provider this call is a guaranteed no-op and pure overhead on a hot path.

SSLEngineImpl.getSession() is not synchronized (checked the bytecode), so this is not a lock cost, but it is still worth guarding. A cheap precondition that keeps SunJSSE on the fast path: on underflow the unwrap consumed nothing, so growth can only be needed when the input buffer is completely full -

case BUFFER_UNDERFLOW =>
  flushToUser()
  if (transportInBuffer.remaining() == transportInBuffer.capacity()) ensureInboundBufferSizes()

(For the BUFFER_OVERFLOW branch the equivalent signal is userOutBuffer being full, which has to be sampled before flushToUser() clears it.)

[He-Pin]

transportPacketSize is raised here but transportOutBuffer is never grown to match - it only grows reactively on BUFFER_OVERFLOW in doWrap.

Two outbound decisions key off transportPacketSize:

• canContinueWrapping requires transportOutBuffer.remaining >= transportPacketSize (line 427);
• shouldFlushTransportNow requires transportOutBuffer.position() >= transportPacketSize (line 562).

Once transportPacketSize exceeds transportOutBuffer.capacity(), both are permanently false, so wrap batching and size-triggered flushing are silently disabled for the lifetime of that connection and it degrades to one record per ByteString. That is stronger than what the new stream-io.md paragraph promises ("can reduce batching, which is bounded by the actual transport buffer capacity").

Consider growing transportOutBuffer here as well - flushing first if it currently holds deferred ciphertext - or capping transportPacketSize at transportOutBuffer.capacity() so the two stay in step.

[He-Pin]

These use math.max(current, applicationSize) - a single record - whereas allocateBuffers (lines 141-143) and growTransportOutBuffer (lines 616-618) derive the same two fields from applicationBufferSize(applicationSize, packetSize), i.e. applicationSize * recordsPerEngineCall.

It is harmless today because this is a max and the batch value is normally larger, but having two formulas for one quantity is a trap for the next reader: if a session's application size ever grew past the batch value, maxUserInBufferSize would silently stop being a batch size and become a single-record cap. Consider deriving both from applicationBufferSize here too.

[He-Pin]

The inner else looks unreachable with the shipped pool: DirectByteBufferPool.takeBufferFromPool only ever returns buffers of defaultBufferSize (DirectByteBufferPool.scala:51-66), and pooledBufferSize is tcp.Settings.DirectBufferSize (Tcp.scala:667), so the outer requiredCapacity <= pooledBufferSize guard already implies buffer.capacity() >= requiredCapacity.

Fine to keep as defence - just noting it cannot be covered by a test unless someone swaps in a different BufferPool.

The important part of this change is the companion guard in releaseTransportBuffer (line 722), and it deserves more credit than it currently gets: DirectByteBufferPool.release performs no validation (DirectByteBufferPool.scala:68-82) - it stores whatever it is handed and acquire() returns it after a clear(). Without the isDirect && capacity() == pooledBufferSize check, every heap fallback buffer allocated by this method would be released into the shared TCP pool and then handed out to unrelated TCP connections for the lifetime of the ActorSystem. Worth stating that explicitly in the PR description, because as written it reads like a cosmetic tidy-up.

[He-Pin]

This changes the environment of the three pre-existing tests in this spec, not just the new ones.

applicationBufferSize divides the transport buffer capacity by packetSize, so with the default direct-buffer-size = 128 KiB the pre-existing tests ran with 131072 / 16709 = 7 -> min(4, 7) = 4 -> 16704 * 4 = 66816 for maxUserInBufferSize/maxUserOutBufferSize. At 18432 that becomes 18432 / 16709 = 1 -> 16704, a 4x reduction in the batching limit for tests that have nothing to do with buffer sizing.

I can see why the value is needed: the two "grow inbound buffers after session sizes increase" cases require the initial buffer to be smaller than negotiatedPacketSize = 32768, otherwise ensureReadBufferCapacity is a no-op and underflowUnwraps/overflowUnwraps stay at 0. But TlsGraphStageSmallBufferSpec (8192) already exists for exactly that purpose. Suggest moving those two cases there - or into a third dedicated spec - and leaving this config with only direct-buffer-pool-limit = 8, which is the part the pool-uniqueness assertion genuinely needs (at the default limit of 1000 it would acquire 128 MiB of direct memory).

If you would rather keep it here, please add a comment explaining the coupling; right now a future reader tuning this value has no way to know it changes batching semantics for unrelated tests.

[He-Pin]

bufferRetries is incremented on every post-handshake scatter unwrap and is never reset, so it really counts "unwraps since the handshake", not retries. A future case that legitimately needs more than 8 unwrap rounds will fail with the misleading "TLS buffers were not resized after repeated retries".

Suggest renaming to something like postHandshakeUnwraps, and/or resetting it once an unwrap returns neither BUFFER_OVERFLOW nor BUFFER_UNDERFLOW. That keeps the intent (fail fast if the stage never resizes) without coupling the guard to the total round count.

[He-Pin]

Unrelated churn, although it does move the file onto the repo convention - TlsSpec.scala, TcpSpec.scala and TlsGraphStageEdgeCasesSpec.scala all put com.typesafe.config after the pekko group, and this file was the outlier. Fine either way; flagging it only against the AGENTS.md "do not mix formatting churn" rule in case a maintainer would rather it stayed out of this PR.

[He-Pin]

#3727 inserts a different paragraph at this exact position, so whichever PR merges second conflicts here and both paragraphs need to be kept.

On the content: it is accurate, and this is the only place in either PR that names pekko.stream.materializer.tls.engine, which is helpful. One nit - "can reduce batching, which is bounded by the actual transport buffer capacity" understates the effect: once the session's packet size exceeds the transport buffer capacity, canContinueWrapping and the size-based flush in shouldFlushTransportNow are both permanently false, so batching is not merely reduced but switched off. See the inline note on ensureInboundBufferSizes.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

[pjfanning]

lgtm