Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
spring-issuemaster opened this issue Jan 16, 2017 · 3 comments

Comments

@spring-issuemaster
Copy link
Collaborator

commented Jan 16, 2017

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:

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:

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

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

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 16, 2017

Juergen Hoeller commented

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

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 17, 2017

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-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 17, 2017

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.