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

@TransactionalEventListener cannot work with TaskExecutor #25129

Closed
weaselmetal opened this issue May 26, 2020 · 11 comments
Closed

@TransactionalEventListener cannot work with TaskExecutor #25129

weaselmetal opened this issue May 26, 2020 · 11 comments
Labels
status: invalid An issue that we don't feel is valid

Comments

@weaselmetal
Copy link

Affects: Spring 4.2

I came to believe that a @TransactionalEventListener cannot work with a applicationEventMulticaster bean that uses a taskExecutor that invokes the listener call in a new / different thread.
Inside a new thread, which cannot be transaction synchronized in my understanding, the call to the listener will be discarded, unless the @TransactionalEventListener(fallbackExecution = true) is set. So, proper coupling to a transaction seems to be impossible.

Should you desire a true asynchronous invocation on other event handlers, you will have to provide a task executor that delegates the invocation to different threads.

JavaDoc doesn't indicate that @TransactionalEventListener cannot work with a non-synchronous task executor in place. But, I might be doing something wrong after all.
As far as I can tell, the code in spring-tx library behaves as it should, but can only work when no task executor is used at all in the event multicaster, or an executor like org.springframework.core.task.SyncTaskExecutor is used.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label May 26, 2020
@snicoll
Copy link
Member

snicoll commented May 26, 2020

What TransactionPhase do you use for this?

@snicoll snicoll added the status: waiting-for-feedback We need additional information before we can continue label May 26, 2020
@weaselmetal
Copy link
Author

I wanted to use BEFORE_COMMIT, but I believe that doesn't matter. Only fallbackExecution = true will have my event handler invoked. I made sure that the event is fired from within a transactional scope.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels May 26, 2020
@snicoll
Copy link
Member

snicoll commented May 26, 2020

I wanted to use BEFORE_COMMIT, but I believe that doesn't matter.

It does matter. A transaction, in essence, is request (thread-) scoped. If you run in the BEFORE_COMMIT propagation phase, you must be running in the transaction and you can't do that in a separate thread.

@weaselmetal
Copy link
Author

weaselmetal commented May 26, 2020

Then I won't argue :D.
What I'm quite certain about is that in org.springframework.transaction.event.ApplicationListenerMethodTransactionalAdapter#onApplicationEvent
for a new thread from a task executor the TransactionSynchronizationManager.isSynchronizationActive() won't evaluate to true. At least that's what I debugged.

@snicoll
Copy link
Member

snicoll commented May 26, 2020

Yes, it won't, for the reason I've exposed (the context, in the new thread, does not have a transaction at all). Considering that you should be able to impact the transaction at that phase, I am going to close this issue now.

@snicoll snicoll closed this as completed May 26, 2020
@snicoll snicoll added status: invalid An issue that we don't feel is valid and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged or decided on labels May 26, 2020
@weaselmetal
Copy link
Author

Wow, that was quick. What you say makes sense. What I was trying to say, though, is:
I would have to create my own applicationEventMulticaster bean, that ensures that any event handler is invoked on the very same thread, as the one that raises the event. I could do so by either not setting any task executor, or a task executor like SyncTaskExecutor.
If that's how it's supposed to be, then at least the documentation should point into that direction.
To me, it doesn't appear that the idea is that I create a applicationEventMulticaster (or a similar) bean, because the creation of that bean is pretty baked into the framework initialization.

@snicoll
Copy link
Member

snicoll commented May 26, 2020

The default behaviour is to invoke events on the same thread so if you don't do anything you'll get the expected result.

I could do so by either not setting any task executor

I am not sure what you mean by that but if you are configuring the event multicaster yourself and you want to be able to handle transactional events, then, yes, it must run on the same thread.

@weaselmetal
Copy link
Author

weaselmetal commented May 26, 2020

Hum, I cannot confirm that.

Method org.springframework.context.support.AbstractApplicationContext#publishEvent(java.lang.Object, org.springframework.core.ResolvableType)
will call org.springframework.context.event.SimpleApplicationEventMulticaster#multicastEvent(org.springframework.context.ApplicationEvent, org.springframework.core.ResolvableType)
which will delegate the listener invocation to a task executor

@Override
public void multicastEvent(final ApplicationEvent event, ResolvableType eventType) {
	ResolvableType type = (eventType != null ? eventType : resolveDefaultEventType(event));
	for (final ApplicationListener<?> listener : getApplicationListeners(event, type)) {
		Executor executor = getTaskExecutor();
		if (executor != null) {
			executor.execute(new Runnable() {
				@Override
				public void run() {
					invokeListener(listener, event);
				}
			});
		}
		else {
			invokeListener(listener, event);
		}
	}
}

Spring sets a task executor for this bean (of type java.util.concurrent.ThreadPoolExecutor), when the bean is created. Why it does so I cannot say (perhaps because we also have a few @Async event handlers?).
So, without any explicit configuration of mine, that's what I get. We seem to agree that in this case a @TransactionalEventListener won't work (under all circumstances).

@snicoll
Copy link
Member

snicoll commented May 26, 2020

Spring sets a task executor for this bean

I don't think that's accurate.

So, without any explicit configuration of mine, that's what I get.

Please share a small project (github repo or zip we can download) that reproduces what you've described.

@weaselmetal
Copy link
Author

weaselmetal commented May 26, 2020

I can't provide a sample project quickly, but this is the callstack when I end up in the setter that sets the task executor:

setTaskExecutor:81, SimpleApplicationEventMulticaster (org.springframework.context.event)
invoke0:-1, NativeMethodAccessorImpl (sun.reflect)
invoke:62, NativeMethodAccessorImpl (sun.reflect)
invoke:43, DelegatingMethodAccessorImpl (sun.reflect)
invoke:498, Method (java.lang.reflect)
setValue:345, BeanWrapperImpl$BeanPropertyHandler (org.springframework.beans)
setPropertyValue:454, AbstractNestablePropertyAccessor (org.springframework.beans)
setPropertyValue:280, AbstractNestablePropertyAccessor (org.springframework.beans)
setPropertyValues:95, AbstractPropertyAccessor (org.springframework.beans)
setPropertyValues:75, AbstractPropertyAccessor (org.springframework.beans)
applyPropertyValues:1519, AbstractAutowireCapableBeanFactory (org.springframework.beans.factory.support)
populateBean:1231, AbstractAutowireCapableBeanFactory (org.springframework.beans.factory.support)
doCreateBean:551, AbstractAutowireCapableBeanFactory (org.springframework.beans.factory.support)
createBean:482, AbstractAutowireCapableBeanFactory (org.springframework.beans.factory.support)
getObject:306, AbstractBeanFactory$1 (org.springframework.beans.factory.support)
getSingleton:230, DefaultSingletonBeanRegistry (org.springframework.beans.factory.support)
doGetBean:302, AbstractBeanFactory (org.springframework.beans.factory.support)
getBean:202, AbstractBeanFactory (org.springframework.beans.factory.support)
initApplicationEventMulticaster:737, AbstractApplicationContext (org.springframework.context.support)
refresh:532, AbstractApplicationContext (org.springframework.context.support)
configureAndRefreshWebApplicationContext:444, ContextLoader (org.springframework.web.context)
initWebApplicationContext:326, ContextLoader (org.springframework.web.context)
contextInitialized:107, ContextLoaderListener (org.springframework.web.context)
callContextInitialized:782, ContextHandler (org.eclipse.jetty.server.handler)
callContextInitialized:424, ServletContextHandler (org.eclipse.jetty.servlet)
startContext:774, ContextHandler (org.eclipse.jetty.server.handler)
startContext:249, ServletContextHandler (org.eclipse.jetty.servlet)
startContext:1242, WebAppContext (org.eclipse.jetty.webapp)
doStart:717, ContextHandler (org.eclipse.jetty.server.handler)
doStart:494, WebAppContext (org.eclipse.jetty.webapp)
start:64, AbstractLifeCycle (org.eclipse.jetty.util.component)
doStart:95, HandlerWrapper (org.eclipse.jetty.server.handler)
doStart:282, Server (org.eclipse.jetty.server)
start:64, AbstractLifeCycle (org.eclipse.jetty.util.component)

@snicoll
Copy link
Member

snicoll commented May 26, 2020

Thanks but that doesn't help. If no configuration on your end is required, you should be able to replicate that in a mini sample.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: invalid An issue that we don't feel is valid
Projects
None yet
Development

No branches or pull requests

3 participants