Event handling in JobExecutionExitCodeGenerator is not thread-safe #30705
Conversation
…herTests, HibernateJpaAutoConfigurationTests threadsafe
spring-projects-issues
added
the
status: waiting-for-triage
There was a problem hiding this comment.
Thanks very much for the PR, @dugenkui03. While I think the change to ClassPathFileSystemWatcherTests is worth making, I'd like to focus on JobExecutionExitCodeGenerator as that's a bug in main code.
| @@ -600,7 +600,7 @@ EventCapturingApplicationListener jpaUsingApplicationListener(EntityManagerFacto | |||
|
|
|||
| static class EventCapturingApplicationListener implements ApplicationListener<ApplicationEvent> { | |||
|
|
|||
| private final List<ApplicationEvent> events = new ArrayList<>(); | |||
There was a problem hiding this comment.
I don't think this change is necessary. Within the context of the two tests that use the listener, events are only ever delivered on the main thread.
| @@ -116,7 +117,7 @@ Listener listener() { | |||
|
|
|||
| static class Listener implements ApplicationListener<ClassPathChangedEvent> { | |||
|
|
|||
| private List<ClassPathChangedEvent> events = new ArrayList<>(); | |||
| @@ -31,7 +31,7 @@ | |||
| */ | |||
| public class JobExecutionExitCodeGenerator implements ApplicationListener<JobExecutionEvent>, ExitCodeGenerator { | |||
|
|
|||
| private final List<JobExecution> executions = new ArrayList<>(); | |||
There was a problem hiding this comment.
👍 This could also break with a single-threaded event multi-caster if getExitCode() is called one one thread when another thread is delivering an event.
…herTests threadsafe
|
@wilkinsona Thanks for your review. Code in |
wilkinsona
changed the title
ConditionEvaluationDeltaLoggingListener, ClassPathFileSystemWatcherTests, HibernateJpaAutoConfigurationTests threadsafe
wilkinsona
added
type: bug
|
Thanks very much, @dugenkui03. I separated out into a separate issue (#30844) the changes to ClassPathFileSystemWatcherTests as, unlike the problem with JobExecutionExitCodeGenereator, the problem with the tests won't affect users. |
The listener could be invoked concurrently by multiple thread, we should make them threadsafe.
SimpleApplicationEventMulticaster doc: