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

Allow TransactionOutboxImpl#scheduler and ExecutorSubmitter#executor programmatic shutdown #687

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

reda-alaoui
Copy link
Contributor

@reda-alaoui reda-alaoui commented Aug 12, 2024

Fix #686

Makes TransactionOutbox and Submitter implement AutoCloseable.

Please note that if any instance is declared as a Spring bean, Spring will automatically call the close method on shutdown, which is very convenient.

@reda-alaoui reda-alaoui changed the title Allow TransactionOutbox#scheduler shutdown Allow TransactionOutboxImpl#scheduler and ExecutorSubmitter#executor programmatic shutdown Aug 12, 2024
@badgerwithagun
Copy link
Member

@reda-alaoui I don't understand why the change to ExecutorSubmitter.

Currently it doesn't control the lifetime of the supplied Executor; your application does. You can easily shut your executor down without needing TransactionOutboxImpl to do it - in fact it seems quite strange to me to expect it to do so. Maybe I'm missing something?

I think the right answer for the scheduler in TransactionOutboxImpl is to go for the same approach: make it a builder option when creating the TransactionOutbox, which defaults to the same behaviour as currently.

@reda-alaoui
Copy link
Contributor Author

reda-alaoui commented Aug 21, 2024

@badgerwithagun,

Do you consider like me that the Submitter should be AutoCloseable ? Considering that the answer is yes, I apply the following rule: any AutoCloseable instance B created by another AutoCloseable instance A should be closed by instance A when instance A is closed. IMO, without this, apis involving AutoCloseable are not consistent and are error prone.

@badgerwithagun
Copy link
Member

I don't see why the Executorubmitter should be autocloseable, no - it doesn't manage any resource lifetimes.

Any Executor it works with is created outside and thus should have its lifetime managed outside.

A good rule of thumb is that whatever creates a resource should close it. ExecutorSubmitter doesn't create any resources.

0L,
TimeUnit.MILLISECONDS,
new ArrayBlockingQueue<Runnable>(16384)));
return ExecutorSubmitter.builder().build();
}
Copy link
Contributor Author

@reda-alaoui reda-alaoui Aug 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@badgerwithagun ExecutorSubmitter does not create an executor, but Submitter does here. AutoCloseable was originally added to Submitter because of this.

Copy link
Member

@badgerwithagun badgerwithagun Aug 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But you don't need to use this method.

You can just create a TransactionOutbox with your own.

var outbox = TransactionOutbox.builder()
   .submitter(ExecutorSubmitter.builder().executor(yourExecutor).build())
   ...
   .builder();

This is almost always what you do in a managed environment. The default Submitter implementation uses ForkJoinPool, which is a very bad idea when running in an application container.

Copy link
Member

@badgerwithagun badgerwithagun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TransactionOutboxImpl#scheduler can't be shutdown without stopping the whole JVM
2 participants