pjfanning commented on code in PR #3727:
URL: https://github.com/apache/pekko/pull/3727#discussion_r4406919101
##########
stream-tests/src/test/scala/org/apache/pekko/stream/io/TcpSpec.scala:
##########
@@ -923,7 +923,14 @@ class TcpSpec extends StreamSpec("""
Flow[ByteString].mapConcat(_.utf8String.toList).takeWhile(_ !=
'\n').map(c => ByteString(c)),
address.getHostName,
address.getPort,
- () => createSSLEngine(TLSRole.server))
+ () => createSSLEngine(TLSRole.server),
Review Comment:
This was the only test calling the 4-argument `bindAndHandleWithTls`
overload, which takes the default `IgnoreComplete` path where the data loss
happens. No other test calls `bindAndHandleWithTls` or `bindWithTls`, so after
this change a truncation regression on the default path, or a broken 4-arg
overload, would pass CI. The enclosing test group is still named "TLS client
and server convenience methods". If the test has to change while the engine is
fixed, please keep a test of the default overload as well, e.g. as pending or
specific to `graph-stage`, rather than replacing it.
##########
stream-tests/src/test/scala/org/apache/pekko/stream/io/TcpSpec.scala:
##########
@@ -923,7 +923,14 @@ class TcpSpec extends StreamSpec("""
Flow[ByteString].mapConcat(_.utf8String.toList).takeWhile(_ !=
'\n').map(c => ByteString(c)),
address.getHostName,
address.getPort,
- () => createSSLEngine(TLSRole.server))
+ () => createSSLEngine(TLSRole.server),
+ backlog = 100,
Review Comment:
`backlog = 100`, `idleTimeout = Duration.Inf` and `verifySession = _ =>
Success(())` copy Tcp's internal defaults by hand. If `Tcp.defaultBacklog` or
the default session verification changes, this test drifts from the overload it
stands in for. `Tcp.defaultBacklog` is `private[pekko]`, so the test can use it.
##########
stream-tests/src/test/scala/org/apache/pekko/stream/io/TcpSpec.scala:
##########
@@ -923,7 +923,14 @@ class TcpSpec extends StreamSpec("""
Flow[ByteString].mapConcat(_.utf8String.toList).takeWhile(_ !=
'\n').map(c => ByteString(c)),
address.getHostName,
address.getPort,
- () => createSSLEngine(TLSRole.server))
+ () => createSSLEngine(TLSRole.server),
+ backlog = 100,
+ options = Nil,
+ idleTimeout = Duration.Inf,
+ verifySession = _ => scala.util.Success(()),
+ // takeWhile cancels its input before the response has necessarily
been encrypted.
+ // Finish sending the response before completing the TLS plaintext
input closes the connection.
+ closing = IgnoreCancel)
Review Comment:
With `IgnoreCancel`, cancelling the user output no longer closes outbound,
so teardown depends only on the handler's completion reaching the TLS stage.
The client never completes (`Source.maybe`), so if a regression stops that
completion from arriving, the test hangs until the 10s patience timeout and
fails with an unclear `futureValue` timeout. The old policy failed fast with a
visible `hell`.
##########
stream-tests/src/test/scala/org/apache/pekko/stream/io/TcpSpec.scala:
##########
@@ -923,7 +923,14 @@ class TcpSpec extends StreamSpec("""
Flow[ByteString].mapConcat(_.utf8String.toList).takeWhile(_ !=
'\n').map(c => ByteString(c)),
address.getHostName,
address.getPort,
- () => createSSLEngine(TLSRole.server))
+ () => createSSLEngine(TLSRole.server),
+ backlog = 100,
+ options = Nil,
+ idleTimeout = Duration.Inf,
+ verifySession = _ => scala.util.Success(()),
+ // takeWhile cancels its input before the response has necessarily
been encrypted.
+ // Finish sending the response before completing the TLS plaintext
input closes the connection.
Review Comment:
This comment is hard to parse ("Finish sending the response before
completing the TLS plaintext input closes the connection") and doesn't explain
the mechanism. The real reason is that the GraphStage engine batches small
writes on a timer and drops them if the user output is cancelled first. If the
workaround stays, the comment should say that and point to an issue, so nobody
copies `IgnoreCancel` into production code as the "right" pattern.
--
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]
