pjfanning commented on code in PR #3703:
URL: https://github.com/apache/pekko/pull/3703#discussion_r4367777153
##########
actor-tests/src/test/scala/org/apache/pekko/event/LoggerSpec.scala:
##########
@@ -238,6 +260,23 @@ class LoggerSpec extends AnyWordSpec with Matchers {
}
}
+ "An actor system with an unresolvable logger configured after a valid one"
must {
+
+ "fail to start without creating any logger actor" in {
+ lifecycleLoggerStarted.set(false)
+ lifecycleLoggerStopped.set(false)
+
+ intercept[ConfigurationException] {
+ ActorSystem("unresolvableLogger", unresolvableLoggerConfig)
+ }
+
+ // the valid logger must not have been created as a system actor that
then
+ // leaks, because class resolution now happens before any logger is
created
+ lifecycleLoggerStarted.get() should ===(false)
+ lifecycleLoggerStopped.get() should ===(false)
Review Comment:
When `lifecycleLoggerStarted` is `false`, this assertion always holds, so it
adds nothing. The test checks call order (the logger was never started) rather
than that nothing leaks, so a leak elsewhere, e.g. in the rollback path,
wouldn't fail it.
##########
actor-tests/src/test/scala/org/apache/pekko/event/LoggerSpec.scala:
##########
@@ -238,6 +260,23 @@ class LoggerSpec extends AnyWordSpec with Matchers {
}
}
+ "An actor system with an unresolvable logger configured after a valid one"
must {
+
+ "fail to start without creating any logger actor" in {
+ lifecycleLoggerStarted.set(false)
+ lifecycleLoggerStopped.set(false)
+
+ intercept[ConfigurationException] {
Review Comment:
This test only exercises the resolve-first path; nothing exercises the new
rollback branch in `Logging.scala`. A test with `[LifecycleRecordingLogger, <a
logger that never replies LoggerInitialized>]` that asserts both actors end up
stopped would cover it, and would fail today because of the issue noted on
`Logging.scala:158`.
Also, `intercept[ConfigurationException]` passes whatever fails, because the
outer catch at `Logging.scala:205` wraps every `Exception` in a
`ConfigurationException`.
##########
actor/src/main/scala/org/apache/pekko/event/Logging.scala:
##########
@@ -134,25 +134,45 @@ trait LoggingBus extends ActorEventBus {
case Nil => classOf[DefaultLogger].getName :: Nil
case loggers => loggers
}
- val myloggers =
- for {
- loggerName <- defaultLoggers
- if loggerName != StandardOutLogger.getClass.getName
- } yield {
- system.dynamicAccess
- .getClassFor[Actor](loggerName)
- .map { actorClass =>
- addLogger(system, actorClass, level, logName)
- }
- .recover {
- case e =>
- throw new ConfigurationException(
- "Logger specified in config can't be loaded [" + loggerName +
- "] due to [" + e.toString + "]",
- e)
+ val loggerNames = defaultLoggers.filterNot(_ ==
StandardOutLogger.getClass.getName)
+ // resolve all logger classes before creating any logger actor, so that
a class
+ // that can't be loaded doesn't leak the already started loggers of
earlier entries:
+ // the `loggers` field, which stopDefaultLoggers iterates, is only
assigned after
+ // all loggers have been created
+ val loggerClasses = loggerNames.map { loggerName =>
+ system.dynamicAccess
+ .getClassFor[Actor](loggerName)
+ .recover {
+ case e =>
+ throw new ConfigurationException(
+ "Logger specified in config can't be loaded [" + loggerName +
+ "] due to [" + e.toString + "]",
+ e)
+ }
+ .get
+ }
+ val myloggers = {
+ val created = List.newBuilder[ActorRef]
+ try {
+ loggerClasses.foreach { actorClass =>
+ created += addLogger(system, actorClass, level, logName)
Review Comment:
The logger that actually fails is never cleaned up. `addLogger` creates the
actor with `systemActorOf` (line 242) before the `InitializeLogger` handshake.
If the reply isn't `LoggerInitialized`, or the ask times out (which also ends
at the throw at line 257 with `"[TIMEOUT]"`), it throws before returning, so
that actor is never added to `created` and the rollback below doesn't stop it.
With `pekko.loggers = [GoodLogger, BadLogger]`, `GoodLogger` is stopped but
`BadLogger`'s actor stays alive until the system is torn down, and that is the
`LoggerInitializationException` case the PR description mentions.
Two possible fixes:
- Have `addLogger` stop its own actor before throwing
`LoggerInitializationException`, and do the same for any other failure after
`systemActorOf`. That keeps the cleanup next to the creation.
- Or, more simply overall: append each logger to `loggers` (under `guard`)
as soon as `addLogger` returns, instead of assigning the field only after all
succeed. `stopDefaultLoggers` then sees the partial state, and since a startup
failure already terminates the system, the separate rollback block isn't
needed. That avoids a second teardown loop that duplicates `stopDefaultLoggers`
(lines 222-228) and can drift from it; that duplication is how this case was
missed.
With the first option, the failing actor still needs stopping in `addLogger`
either way.
##########
actor/src/main/scala/org/apache/pekko/event/Logging.scala:
##########
@@ -134,25 +134,45 @@ trait LoggingBus extends ActorEventBus {
case Nil => classOf[DefaultLogger].getName :: Nil
case loggers => loggers
}
- val myloggers =
- for {
- loggerName <- defaultLoggers
- if loggerName != StandardOutLogger.getClass.getName
- } yield {
- system.dynamicAccess
- .getClassFor[Actor](loggerName)
- .map { actorClass =>
- addLogger(system, actorClass, level, logName)
- }
- .recover {
- case e =>
- throw new ConfigurationException(
- "Logger specified in config can't be loaded [" + loggerName +
- "] due to [" + e.toString + "]",
- e)
+ val loggerNames = defaultLoggers.filterNot(_ ==
StandardOutLogger.getClass.getName)
+ // resolve all logger classes before creating any logger actor, so that
a class
+ // that can't be loaded doesn't leak the already started loggers of
earlier entries:
+ // the `loggers` field, which stopDefaultLoggers iterates, is only
assigned after
+ // all loggers have been created
+ val loggerClasses = loggerNames.map { loggerName =>
+ system.dynamicAccess
+ .getClassFor[Actor](loggerName)
+ .recover {
+ case e =>
+ throw new ConfigurationException(
+ "Logger specified in config can't be loaded [" + loggerName +
+ "] due to [" + e.toString + "]",
+ e)
+ }
+ .get
+ }
+ val myloggers = {
+ val created = List.newBuilder[ActorRef]
+ try {
+ loggerClasses.foreach { actorClass =>
+ created += addLogger(system, actorClass, level, logName)
+ }
+ created.result()
+ } catch {
+ case NonFatal(e) =>
Review Comment:
`addLogger` blocks in `Await.result`, and `InterruptedException` isn't
matched by `NonFatal`. If the thread creating the `ActorSystem` is interrupted
while waiting for an `InitializeLogger` reply, this rollback is skipped and the
loggers created so far are neither unsubscribed nor stopped. The outer `case e:
Exception` (line 205) then wraps it in a `ConfigurationException`, which also
hides the interrupt. Catching `Throwable` here, or doing the cleanup in a
`finally` guarded by a success flag, would cover it.
##########
actor/src/main/scala/org/apache/pekko/event/Logging.scala:
##########
@@ -134,25 +134,45 @@ trait LoggingBus extends ActorEventBus {
case Nil => classOf[DefaultLogger].getName :: Nil
case loggers => loggers
}
- val myloggers =
- for {
- loggerName <- defaultLoggers
- if loggerName != StandardOutLogger.getClass.getName
- } yield {
- system.dynamicAccess
- .getClassFor[Actor](loggerName)
- .map { actorClass =>
- addLogger(system, actorClass, level, logName)
- }
- .recover {
- case e =>
- throw new ConfigurationException(
- "Logger specified in config can't be loaded [" + loggerName +
- "] due to [" + e.toString + "]",
- e)
+ val loggerNames = defaultLoggers.filterNot(_ ==
StandardOutLogger.getClass.getName)
+ // resolve all logger classes before creating any logger actor, so that
a class
+ // that can't be loaded doesn't leak the already started loggers of
earlier entries:
+ // the `loggers` field, which stopDefaultLoggers iterates, is only
assigned after
+ // all loggers have been created
+ val loggerClasses = loggerNames.map { loggerName =>
+ system.dynamicAccess
+ .getClassFor[Actor](loggerName)
+ .recover {
+ case e =>
+ throw new ConfigurationException(
+ "Logger specified in config can't be loaded [" + loggerName +
+ "] due to [" + e.toString + "]",
+ e)
+ }
+ .get
+ }
+ val myloggers = {
+ val created = List.newBuilder[ActorRef]
+ try {
+ loggerClasses.foreach { actorClass =>
+ created += addLogger(system, actorClass, level, logName)
Review Comment:
The logger that actually fails is never cleaned up. `addLogger` creates the
actor with `systemActorOf` (line 242) before the `InitializeLogger` handshake.
If the reply isn't `LoggerInitialized`, or the ask times out (which also ends
at the throw at line 257 with `"[TIMEOUT]"`), it throws before returning, so
that actor is never added to `created` and the rollback below doesn't stop it.
With `pekko.loggers = [GoodLogger, BadLogger]`, `GoodLogger` is stopped but
`BadLogger`'s actor stays alive until the system is torn down, and that is the
`LoggerInitializationException` case the PR description mentions.
Two possible fixes:
- Have `addLogger` stop its own actor before throwing
`LoggerInitializationException`, and do the same for any other failure after
`systemActorOf`. That keeps the cleanup next to the creation.
- Or, more simply overall: append each logger to `loggers` (under `guard`)
as soon as `addLogger` returns, instead of assigning the field only after all
succeed. `stopDefaultLoggers` then sees the partial state, and since a startup
failure already terminates the system, the separate rollback block isn't
needed. That avoids a second teardown loop that duplicates `stopDefaultLoggers`
(lines 222-228) and can drift from it; that duplication is how this case was
missed.
Note that with the second option the failing actor is still never added to
`loggers`, so `addLogger` must stop it before throwing either way.
--
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]
