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

Revise an ExecutorService injections in favor of injected TaskExecutor #8642

Closed
artembilan opened this issue Jun 8, 2023 · 4 comments · Fixed by #8647
Closed

Revise an ExecutorService injections in favor of injected TaskExecutor #8642

artembilan opened this issue Jun 8, 2023 · 4 comments · Fixed by #8647
Assignees
Milestone

Comments

@artembilan
Copy link
Member

Samples are PostgresChannelMessageTableSubscriber, org.springframework.integration.hazelcast.leader.LeaderInitiator.
These threads potentially can be created as virtual threads with Java 21.

@artembilan artembilan added this to the 6.2.x milestone Jun 8, 2023
@garyrussell
Copy link
Contributor

Consider using a TaskExecutor instead of a TF directly.

I tested virtual threads with spring-kafka with this...

	@Bean
	ApplicationRunner runner(KafkaTemplate<String, String> template,
			ConcurrentKafkaListenerContainerFactory<?, ?> factory) {

		ThreadFactory tFactory = Thread.ofVirtual().factory();
		factory.getContainerProperties().setListenerTaskExecutor(new SimpleAsyncTaskExecutor(tFactory));
		return args -> {
			template.send("loom1", "test");
			template.send("loom1", "test");
			template.send("loom1", "test");
			template.send("loom1", "test");
			Thread.sleep(5000);
		};
	}

@artembilan
Copy link
Member Author

What I don't like with a single managed TaskExecutor that threads in different components are going to have same prefix in their names.
It is a bit inconvenient when you trace the logic in the logs.
With the name of a thread dedicated to the specific component it is very clear what is going on.

Probably the whole paradigm of managed executor doesn't bring any value when we switch to virtual threads.
It might really be convenient from microservice perspective when Spring Boot gives us this or that TaskExecutor automatically, but for more complex and asynchronous applications like with Spring Integration, it is really misleading to use same prefix name for all virtual threads.

We can continue discussion though since I don't claim to be expert in this area: just sharing my pain over years with single managed executor.

Out of context, but still might be an argument for TaskExecutor injection in some places, @tzolov . See, there is an org.springframework.core.task.support.ExecutorServiceAdapter for our convenience when we would like to deal with Future<?> submit().

@garyrussell
Copy link
Contributor

I didn't mean a single shared TE, each component can get its own.

In any case, as I've said before, unless something has changed, VTs don't get a name for logging purposes; I added this property to the listener container:

spring-projects/spring-kafka#2501

@artembilan
Copy link
Member Author

Yeah...
The point is that we talk about those components which uses only a single thread, like LockRegistryLeaderInitiator.
But yeah, for consistency, a single-threaded TE could be used as well.

@artembilan artembilan changed the title Revise an ExecutorService injections in favor of ThreadFactory in places where only single thread is used Revise an ExecutorService injections in favor of injected TaskExecutor Jun 12, 2023
artembilan added a commit to artembilan/spring-integration that referenced this issue Jun 12, 2023
Related to spring-projects#8642

For consistency with other Spring requirements and realignment with virtual threads,
it is better to require a `TaskExecutor` injection instead of `ThreadFactory`

* Fix `DebeziumMessageProducer` to rely on a `TaskExecutor` API instead of `ThreadFactory`
and `ExecutorService`
artembilan added a commit to artembilan/spring-integration that referenced this issue Jun 13, 2023
Fixes spring-projects#8642

* Rework some `Executors.newSingleThreadExecutor()` to `ExecutorServiceAdapter(new SimpleAsyncTaskExecutor())`
* Expose `TaskExecutor` setters; deprecate `ExecutorService`-based
* Some other code clean up in the effected classes: `LogAccessor`, no `synchronized` in critical blocks
* Give a meaningful prefix for default threads in the context of components, e.g. `SubscribableRedisChannel` - `getBeanName() + "-"`
artembilan added a commit that referenced this issue Jun 13, 2023
Related to #8642

For consistency with other Spring requirements and realignment with virtual threads,
it is better to require a `TaskExecutor` injection instead of `ThreadFactory`

* Fix `DebeziumMessageProducer` to rely on a `TaskExecutor` API instead of `ThreadFactory`
and `ExecutorService`

* * Remove unused import from the `DebeziumMessageProducerSpec`
artembilan added a commit to artembilan/spring-integration that referenced this issue Jun 14, 2023
Fixes spring-projects#8642

* Rework some `Executors.newSingleThreadExecutor()` to `ExecutorServiceAdapter(new SimpleAsyncTaskExecutor())`
* Expose `TaskExecutor` setters; deprecate `ExecutorService`-based
* Some other code clean up in the effected classes: `LogAccessor`, no `synchronized` in critical blocks
* Give a meaningful prefix for default threads in the context of components, e.g. `SubscribableRedisChannel` - `getBeanName() + "-"`
garyrussell pushed a commit that referenced this issue Jun 14, 2023
* GH-8642: Revise executors in the project

Fixes #8642

* Rework some `Executors.newSingleThreadExecutor()` to `ExecutorServiceAdapter(new SimpleAsyncTaskExecutor())`
* Expose `TaskExecutor` setters; deprecate `ExecutorService`-based
* Some other code clean up in the effected classes: `LogAccessor`, no `synchronized` in critical blocks
* Give a meaningful prefix for default threads in the context of components, e.g. `SubscribableRedisChannel` - `getBeanName() + "-"`

* * Fix `PostgresChannelMessageTableSubscriberTests` for
`PostgresSubscribableChannel` initialization to let it create
its internal `Executor`

* Use an `AsyncTaskExecutor` injection instead of `ExecutorServiceAdapter` wrapping

* Fix `LockRegistryLeaderInitiatorTests` for `taskExecutor` injection

* Bring back `LockRegistryLeaderInitiator.setExecutorService()`
as an accident after property auto-renaming
@artembilan artembilan modified the milestones: 6.2.x, 6.2.0-M1 Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants