Copilot commented on code in PR #3652:
URL: https://github.com/apache/pekko/pull/3652#discussion_r4361129507
##########
remote/src/test/scala/org/apache/pekko/remote/artery/RemoteConnectionSpec.scala:
##########
@@ -29,16 +34,195 @@ class RemoteConnectionSpec extends
ArteryMultiNodeSpec("pekko.remote.retry-gate-
EventFilter.warning(pattern = "received dead letter.*")))
}
+ private def warningsProbe(system: ActorSystem): TestProbe = {
+ val probe = new TestProbe(system)
+ system.eventStream.subscribe(probe.ref, classOf[Logging.Warning])
+ probe
+ }
Review Comment:
These tests subscribe probes to the `eventStream` but never unsubscribe (or
stop) them. Since `localSystem` is reused across multiple test cases, this can
accumulate subscribers, increase noise/overhead, and make later assertions
harder to reason about. Consider unsubscribing after `expectCollisionWarning`
(or stopping the probe actor so it’s removed from the event stream) to keep
test isolation tight.
##########
slf4j/src/main/scala/org/apache/pekko/event/slf4j/Slf4jLogger.scala:
##########
@@ -75,7 +75,19 @@ class Slf4jLogger extends Actor with SLF4JLogging with
RequiresMessageQueue[Logg
val mdcPekkoUidAttributeName = "pekkoUid"
private def pekkoAddress =
context.system.asInstanceOf[ExtendedActorSystem].provider.addressString
- private val pekkoUid: String =
context.system.asInstanceOf[ExtendedActorSystem].uid.toString
+ // the uid is not available until the remote transport has been started, so
it is resolved lazily
+ private var _pekkoUid: String = ""
+ private def pekkoUid: String = {
+ if (_pekkoUid.isEmpty) {
+ try {
+ _pekkoUid =
context.system.asInstanceOf[ExtendedActorSystem].uid.toString
+ } catch {
+ case _: IllegalStateException =>
+ // when logging before the uid is generated (transport binding when
remoting is enabled)
+ }
+ }
+ _pekkoUid
+ }
Review Comment:
Catching and ignoring all `IllegalStateException` from `uid` lookup can
inadvertently hide real initialization/configuration bugs (since
`ActorSystemImpl.uid` now wraps any `NonFatal` in an `IllegalStateException`).
To avoid suppressing unrelated failures, narrow the catch to the specific
early-access case (e.g., match on a dedicated exception type or a message
predicate) and rethrow other `IllegalStateException`s so they remain visible.
##########
remote/src/main/scala/org/apache/pekko/remote/artery/ArteryTransport.scala:
##########
@@ -329,6 +329,11 @@ private[remote] abstract class ArteryTransport(_system:
ExtendedActorSystem, _pr
override def addresses: Set[Address] = _addresses
override def localAddressForRemote(remote: Address): Address = defaultAddress
+ /**
+ * Must not be accessed before `start()`, will throw NullPointerException
otherwise.
+ */
+ override def systemUid: Long = _localAddress.uid
Review Comment:
`systemUid` currently relies on `_localAddress` being initialized and will
throw a `NullPointerException` if accessed too early (as the comment notes).
This is hard to diagnose and inconsistent with the new `ActorSystemImpl.uid`
behavior, which intends to surface early access as an `IllegalStateException`.
Consider guarding access here and throwing an `IllegalStateException` with a
clear message when `_localAddress` is not initialized yet.
--
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]
