SimpleApplicationEventMulticaster should not rely on ClassCastException having a message [SPR-15145]

[spring-projects-issues]

Mariusz Luciow opened SPR-15145 and commented

SimpleApplicationEventMulticaster in this commit driven by #19412 changed the ClassCastException catch logic to:

catch (ClassCastException ex) {
  if (ex.getMessage().startsWith(event.getClass().getName())) {
    // Possibly a lambda-defined listener which we could not resolve the generic event type for
    LogFactory.getLog(getClass()).debug("Non-matching event type for listener: " + listener, ex);
  } else {
    throw ex;
  }
}

Note that if ex.getMessage() returns NULL whole method will throw NPE.

To speed up execution of methods that frequently throw exceptions, the C2 compiler generates code that uses a pre-built exception (pre-built at compile time). The pre-built exception does not contain neither a stack trace nor a message detailing the exception.

This means that after some time the compiler can replace normal ClassCastException with empty one, without message nor stack trace.

More detailed explanation of this behaviour can be found here:

• openjdk jira
• oracle release notes

There should be a check in place to ensure that message is not null.

---

Affects: 4.3.5

Reference URL: 13001b9#diff-2434dcbaad29ced6a104bb6523c4f67e

Issue Links:

• SimpleApplicationEventMulticaster should not generally suppress ClassCastException [SPR-14846] #19412 SimpleApplicationEventMulticaster should not generally suppress ClassCastException
• SimpleApplicationEventMulticaster does not deal with lambda-defined listeners when ErrorHandler is set [SPR-15838] #20393 SimpleApplicationEventMulticaster does not deal with lambda-defined listeners when ErrorHandler is set
• Lambda error detection might not work on JDK 9 [SPR-16435] #20981 Lambda error detection might not work on JDK 9
• Classification of ClassCastExceptions doesn't work in JDK 11 (OpenJDK) [SPR-17093] #21630 Classification of ClassCastExceptions doesn't work in JDK 11 (OpenJDK)

Referenced from: commits 976d32f, 153fd82, 64d4afa, 0655d73

[spring-projects-issues]

Juergen Hoeller commented

Good catch! Fixed in master and to be backported to 4.3.6.

[spring-projects-issues]

Mariusz Luciow commented

Hi Juergen,

I'm not convinced if the check you've made is a good idea:

catch (ClassCastException ex) {
  String msg = ex.getMessage();
  if (msg != null && msg.startsWith(event.getClass().getName())) {
    // Possibly a lambda-defined listener which we could not resolve the generic event type for
    Log logger = LogFactory.getLog(getClass());
    if (logger.isDebugEnabled()) {
      logger.debug("Non-matching event type for listener: " + listener, ex);
    }
  } else {
    throw ex;
  }
}

Current behaviour will work only until compiler decides to start throwing pre-build exception. After that the catch logic will completely change, throwing exception instead of swallowing it, which may be even harder to debug.
Honestly I'm not sure if this issue can be solved here. Maybe Spring should prohibit registration of ApplicationListener<?> and throw exceptions during startup instead? For example, the correct way to register listener could look like this, if possible:

@Bean
@EventListener(ContextRefreshedEvent.class)
ApplicationListener<ContextRefreshedEvent> contextRefreshedEventApplicationListener() {
    return System.err::print;
}

[spring-projects-issues]

Juergen Hoeller commented

Good point. It's actually closer to our intentions if we turn that check into (msg == null || msg.startsWith(event.getClass().getName())), rather over-swallowing than under-swallowing, closer to how we had it originally. If there is some unrelated ClassCastException involved, developers are going to notice it on first appearance anyway. I'll do that right away.

As for other ways of identifying lambda-defined listeners, we got a few related JIRA tickets already. For the time being, we cannot change the rules there since we're in the middle of the 4.3.x line. I'm afraid we can only fine-tune the current approach.