He-Pin commented on issue #2860: URL: https://github.com/apache/pekko/issues/2860#issuecomment-6482139963
Current-main recheck for #2860 (Pekko `a55a008fdf`, 2026-10-29). This supersedes parts of the older remaining-work list. The TLS GraphStage implementation and the default-engine migration are separate milestones. The new engine is implemented and has substantial regression coverage, but `pekko.stream.materializer.tls.engine` still defaults to `legacy-actor`. This review found a reproducible buffer-allocation/growth regression; the focused fix is #3728. **Already completed** - #2873 removed the dead internal Unzip implementation. - #2874 migrated fanout publisher execution to `FanoutPublisherBridgeStage`; #2910 fixed abrupt-shutdown signaling. - #3027 removed the dead `ActorProcessor`, `ActorProcessorImpl`, and `ExposedPublisherReceive` types. `ActorProcessor.scala` still contains buffering helpers used by legacy TLS. - #3176 removed `FanoutPublisherSink`. Current `Sink.asPublisher(true)` directly composes `Flow[T].async` with the GraphStage bridge. This is no longer a future rewrite. - #2878 added the opt-in TLS GraphStage. #3324 subsequently fixed deferred-output handling on inbound close, and #3323 fixed transport abort on session-verifier rejection. **Review against the original TLSActor state machine** The comparison uses the actual `TransferState` algebra, not just matching phase names: `A || B` completes when both complete, while `A && B` completes when either completes ([Transfer.scala:114](https://github.com/apache/pekko/blob/a55a008fdf/stream/src/main/scala/org/apache/pekko/stream/impl/Transfer.scala#L114)). | Original phase | GraphStage counterpart | Contract to preserve | | --- | --- | --- | | `bidirectional` | `BidirectionalPhase` | Inbound first, then outbound; engine NEED_WRAP can progress without application data. | | `flushingOutbound` | `FlushingOutboundPhase` | Flush protocol output subject to transport demand; handle SSLException without stranding shutdown. | | `awaitingClose` | `AwaitingClosePhase` | Continue consuming peer close records, discarding application output. | | `outboundClosed` | `OutboundClosedPhase` | Continue inbound processing and required handshake/close writes. | | `inboundClosed` | `InboundClosedPhase` | Preserve ignore-cancel behavior while processing the remaining outbound side. | | `completedPhase` / `pumpFinished` | `CompletedPhase` / `StageCompletionPendingFlag` | Cancel inputs, complete outputs, and finish already-pending output before stopping. | The original states are at [TLSActor.scala:233](https://github.com/apache/pekko/blob/a55a008fdf/stream/src/main/scala/org/apache/pekko/stream/impl/io/TLSActor.scala#L233), and the explicit port predicates are at [TlsGraphStage.scala:186](https://github.com/apache/pekko/blob/a55a008fdf/stream/src/main/scala/org/apache/pekko/stream/impl/io/TlsGraphStage.scala#L186). The main new proof obligations are batching, delayed flushes, output slots, and pooled buffers. #3324 is an example of why copying the old phases alone was insufficient. I also checked an early cipher-output cancellation hypothesis with both implementations. With two `Source.maybe` inputs, a server engine waiting for input, `Sink.cancelled` on cipher output, and `Sink.ignore` on plaintext output, the legacy and GraphStage paths behaved alike in all four async/no-async combinations. That experiment does not establish a migration regression, so it is not being presented as one or used to justify a redundant spec. Explicitly selecting GraphStage also exposed a closing-policy assumption in the existing public `TcpSpec` TLS example: its server uses `takeWhile` and expects `hello`, but default `IgnoreComplete` permits the resulting plaintext-output cancellation to discard the last pending byte (`hell` was observed on both unmodified main and the buffer-fix branch). This is allowed by the documented TLS closing contract, not evidence that the state machine must drain arbitrary plaintext on eager cancellation. [SslTlsOptions.scala:85](https://github.com/apache/pekko/blob/a55a008fdf/stream/src/main/scala/org/apache/pekko/stream/SslTlsOptions.scala#L85) describes the server use case for `IgnoreCancel`, and the shared `CancellingRHS` scenario already uses it. #3727 corrects that test's server policy while retaining its full-response and termination assertions; production close semantics stay unchanged. **Confirmed gap: TLS allocation depends on the TCP pool size** [TlsGraphStage.scala:144](https://github.com/apache/pekko/blob/a55a008fdf/stream/src/main/scala/org/apache/pekko/stream/impl/io/TlsGraphStage.scala#L144) acquires three transport buffers from the TCP pool. [acquireTransportBuffer:682](https://github.com/apache/pekko/blob/a55a008fdf/stream/src/main/scala/org/apache/pekko/stream/impl/io/TlsGraphStage.scala#L682) throws if a fixed-size pooled buffer is smaller than `SSLSession.getPacketBufferSize`. Legacy TLS allocates its own buffers independently of TCP. On JDK 21 with `-Dpekko.io.tcp.direct-buffer-size=8192`: - `TlsSpec`: **113 passed**. - `TlsGraphStageSpec`, filtered to `SingleBytes`: **8 failed**, covering TLS 1.2/1.3 and in-memory/TCP communication patterns. - All eight fail at startup: `TLS packet buffer requires 16709 bytes but pekko.io.tcp.direct-buffer-size is 8192`. The differential run used the compiled ScalaTest classes. An equivalent focused sbt reproduction is: ```sh rtk sbt -Dpekko.io.tcp.direct-buffer-size=8192 \ "stream-tests / Test / testOnly org.apache.pekko.stream.io.TlsSpec -- -z SingleBytes" \ "stream-tests / Test / testOnly org.apache.pekko.stream.io.TlsGraphStageSpec -- -z SingleBytes" ``` There is also a confirmed runtime failure even when the configured buffer fits the initial packet size. [growTransportOutBuffer:578](https://github.com/apache/pekko/blob/a55a008fdf/stream/src/main/scala/org/apache/pekko/stream/impl/io/TlsGraphStage.scala#L578) asks the same fixed-size pool for twice the current capacity, which cannot succeed. A second differential probe used real JDK 21 TLS 1.2 engines, completed the handshake, and round-tripped an initial 2,048-byte payload with a 17,000-byte pool. Holding the batching timer then allowed a 1,024-byte wrap to retain 1,053 ciphertext bytes. The next 2,048-byte write encountered only 15,947 free bytes, below the provider's 16,709-byte requirement, so `wrap` returned `BUFFER_OVERFLOW`. GraphStage attempted to obtain 34,000 bytes from the 17,000-byte pool and failed; legacy TLS delivered all 5,120 application bytes. The wrapper only observed real engine results; it did not fabricate the overflow. The focused fix preserves pooled direct buffers when they fit and allocates larger heap buffers when needed, including output growth. Heap buffers are never returned to the fixed-size TCP pool. Three necessary directional cases cover TLS 1.2/1.3 with an 8 KiB pool, pool ownership after stage termination, and overflow while earlier ciphertext remains buffered. All three failed before the fix; the existing deferred-close cases continued to pass. The deterministic overflow case reuses the existing scripted engine/scheduler rather than adding a broad duplicate suite. The default pool uses 128 KiB buffers, so the three held buffers cost approximately **384 KiB of direct memory per live TLS stage**, before application buffers. #3019 proposed heap/on-demand buffers but was closed without merging. That work remains relevant; measure native memory as well as heap allocation and throughput. **Downstream status** Current pekko-http main (`127528261`) has no source references to `TLSActor`, `TlsModule`, `TlsUtils`, or `TlsGraphStage`; HTTP and HTTP/2 use the public `TLS(...)` API. The earlier direct-dependency warning should not be repeated as an unverified current blocker. pekko-http already runs both TLS engines across JDK 17/21/25 and Scala 2.13/3. All **12 jobs** in the [2026-10-28 nightly](https://github.com/apache/pekko-http/actions/runs/40615900881) succeeded. The JDK 21/Scala 2.13/graph-stage log used Pekko `2.0.0-M4+190-2c035294-SNAPSHOT`: this is existing integration evidence, not a downstream rerun against today's exact HEAD. Published downstream versions and other consumers still need checking before class removal. **Suggested 2.0.0 scope** | Priority | Work | Boundary / acceptance criteria | | --- | --- | --- | | P0 | TLS buffer compatibility and targeted lifecycle coverage, then a separate default-switch PR | Preserve both engines' contracts; cross-JDK TLS, HTTP/HTTP2 and Artery validation; native-memory and latency evidence. Keep `legacy-actor` as a transition fallback. | | P1 | Small `Sink.cancelled` / `CancelSink` GraphStage conversion | Preserve cancellation without requesting an element, subscription ordering, `NotUsed`, and attributes. Run `CancelledSinkSubscriberTest` plus focused sink/flow tests. Verify materialization benefit rather than assuming it. | | P1 | Remove unused `FanIn` and `FanOut` actor classes, subject to dependency/MiMa checks | Only the unused classes. Their companion objects still supply `InputBunch` / `OutputBunch` and protocol messages used by TLSActor; do not delete the whole files while fallback TLS remains. | | P2 | Separate investigation of `Flow.fromProcessor(Mat)` / `ProcessorModulePhase` | Preserve one factory call per materialization, the original materialized value, Reactive Streams sequencing/cancellation, and avoid unnecessary additional async boundaries. Prototype and benchmark first. | | Later | `PublisherSource`, `SubscriberSource`, `SubscriberSink`, and single-subscriber `PublisherSink` | These are Reactive Streams boundary adapters, not ordinary actor operators. `PublisherSink` deliberately uses `VirtualPublisher` to delay subscription. A GraphStage wrapper alone may add overhead or alter contracts. | Useful TLS follow-ups before changing the default: - Extend batching, fragmented-record, slow-consumer and timer/cancellation edge cases to TLS 1.3; the dedicated edge-case fixture currently fixes TLS 1.2, although the shared suite covers both protocols. - Keep Artery TLS coverage with GraphStage explicitly selected in the cutover gate; the six cases run here passed on JDK 21, rather than constituting a cross-JDK Artery matrix. - Retain public-API selection coverage in isolated JVMs/CI after #3727. Selection is currently a classloader-global `val` using `ConfigFactory.load()`, not each materializer's config; the selection unit tests exercise precedence helpers. The local TCP and Artery runs here explicitly selected GraphStage. - Improve benchmark scope: [warmRoundTrip:148](https://github.com/apache/pekko/blob/a55a008fdf/bench-jmh/src/main/scala/org/apache/pekko/stream/io/TlsBenchmark.scala#L148) creates a fresh client/server pair for every 1,000-record invocation, despite the class comment saying engines are reused. Add an actual long-lived-connection fixture, TLS 1.3, latency, and live-connection memory measurements. Historical throughput numbers alone do not establish those properties. After a transition release and downstream validation, retire `TlsModule`, its materializer island/phase, and `TLSActor`, then prune the unused buffering/Pump/Transfer chain. First move the still-used `TlsUtils.applySessionParameters` out of `TLSActor.scala`: the GraphStage calls it too. Do not blanket-delete `ActorSubscriberMessage` or `ActorPublisher.scala`: current GraphStage internals use the former; the latter owns shutdown sentinels used by the GraphInterpreter, fanout bridge, subscriber management, and hubs. Actor-ref sources, stream refs, TCP and file/stream I/O adapters are already GraphStages. Interpreter/supervisor actors are runtime infrastructure, outside this operator migration. **Validation** - JDK 21, current Pekko HEAD: `TlsGraphStageDeferredCloseSpec` **3 passed**, then `TlsSpec`, `TlsGraphStageSpec`, `TlsGraphStageEdgeCasesSpec`, `TlsGraphStageIsolatedSpec`, `TlsEngineSelectionSpec`, and `TLSUtilsSpec` **254 passed**: **257 total, 0 failed/ignored/pending** under default buffer configuration. - Small-buffer differential run: **113 legacy passes / 8 GraphStage failures**, as described above. - Legacy/GraphStage early-cancellation differential experiment: matching behavior; not counted as a migration defect. - Fix branch on JDK 21: **260 TLS tests passed**, plus **8/8** small-pool shared communication cases and **6/6** Artery TLS tests with GraphStage explicitly selected. - JDK 17 and 25: all **6/6** focused small-buffer/deferred-close cases passed on each JDK. - Real 17,000-byte overflow probe after the fix: both engines delivered **5,120 bytes**, including successful GraphStage recovery from the observed overflow. - Corrected public TCP TLS case: both engines pass on JDK 21; a separate unchanged-main probe with the corrected server policy passed TLS 1.2/1.3 **10 times each**. - Native scalafmt, header generation, cross-Scala header checks and `git diff --check` passed. `sortImports` hit a scalameta linkage error; unrelated edits were removed. - Full local PR validation/MiMa are delegated to remote CI as requested; an in-progress local Paradox build was stopped before completion. - The focused PRs are initially drafts while final review and remote CI proceed; no default-engine change or legacy-class removal is included. The fix PR addresses this demonstrated regression only. Switching the default, changing the normal buffer-memory policy, and retiring legacy classes should remain separate changes with their own acceptance evidence. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
