pjfanning commented on code in PR #3707:
URL: https://github.com/apache/pekko/pull/3707#discussion_r4371558563
##########
remote-tests/src/test/scala/org/apache/pekko/remote/testconductor/ControllerSpec.scala:
##########
@@ -49,6 +51,34 @@ class ControllerSpec extends
PekkoSpec(ControllerSpec.config) with ImplicitSende
c ! PoisonPill // clean up so network connections don't accumulate
during test run
}
+ "reply with the actually bound socket address" in {
+ val c = system.actorOf(Props(classOf[Controller], 1, new
InetSocketAddress(InetAddress.getLocalHost, 0)))
+ c ! Controller.GetSockAddr
+ val addr = expectMsgType[InetSocketAddress]
+ addr.getPort should not be 0
+ c ! PoisonPill // clean up so network connections don't accumulate
during test run
+ }
+
+ "reply with failure when the conductor port cannot be bound" in {
+ // bind to the same specific address the controller will use, a wildcard
+ // bind would not conflict on all platforms
+ val serverSocket = new ServerSocket(0, 50, InetAddress.getLocalHost)
Review Comment:
Nit: this relies on a second bind to the same address and port failing even
though the conductor server sets `SO_REUSEADDR` on non-Windows platforms. That
holds on Linux and macOS while this socket is listening, and CI is green, but
it's the one place I'd expect possible flakiness on other platforms or CI
hosts. Not a blocker; just flagging it in case it shows up later.
##########
multi-node-testkit/src/main/scala/org/apache/pekko/remote/testconductor/Conductor.scala:
##########
@@ -529,7 +530,16 @@ private[pekko] class Controller(private var
initialParticipants: Int, controller
}
case GetNodes => sender() ! nodes.keys
case GetSockAddr =>
- sender() ! connection.channelFuture.sync().channel().localAddress()
+ val replyTo = sender()
+ // Netty also invokes listeners added after completion, so a failed bind
+ // is reported even when it finishes before the first address query.
+ connection.channelFuture.addListener((future: ChannelFuture) => {
+ val result = Try {
+ if (future.isSuccess) future.channel().localAddress()
+ else throw future.cause()
+ }
+ result.fold(cause => replyTo ! Status.Failure(cause), addr => replyTo
! addr)
Review Comment:
Nit: the `Try { ... else throw future.cause() }.fold(...)` builds an
exception path only to unpack it again, and if `cause` were fatal, `Try` would
rethrow it on the Netty event loop. A direct branch is simpler:
```scala
connection.channelFuture.addListener((future: ChannelFuture) =>
if (future.isSuccess) replyTo ! future.channel().localAddress()
else replyTo ! Status.Failure(future.cause()))
```
That would also drop the `scala.util.Try` import.
--
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]
