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
Add support for transaction bound application events [SPR-12080] #16696
Comments
Jean-Baptiste Nizet commented This is something that CDI provides and that is really missing in Spring, IMHO. Nevertheless, I'd like to suggest another approach for several reasons:
Here's a repo where I implement what I have in mind, and which is quite close to what CDI does. The README explains the design goals and show how to use it. The implementation works fine and should be fast at firing events. I don't know the internals of Spring well enough to know if a bean postprocessor is the best way to support this functionality, though. The implementation depends on Guava, but could easily be modified to have no dependency other than Spring. I just wanted an easy way to have a cache. https://github.com/Ninja-Squad/spring-events The project builds and tests pass with Java 6, 7 and 8. More details are available in the javadoc, that can be generated using |
Oliver Drotbohm commented
Generally speaking, the approach I provided didn't have the goal to copy the CDI eventing mechanism but rather extend the current application event mechanism in an idiomatic way. Trying to force a completely different design down Spring's throat is neither going to work nor likely to be accepted. As indicated in the current model creates dependencies to Spring on the producer and consumer side of things but a mitigating layer could easily be written on top of that. So I think extending the current mechanism with the general capability of hooking into transaction boundaries is a good step. Developers accepting the dependency can the use it as is. Purists trying to avoid it can then build whatever they want on top of that. |
Jean-Baptiste Nizet commented First of all, thanks for reading and considering my comment.
To me, the main point of an event mechanism is to have loose coupling between the producer and the consumers of the event. The producer shouldn't know and care about who the consumers are, and how they need to consume the event, IMHO. Regarding my example, sure this asynchronous event requires after-transaction semantics. But the reverse is not always true: you could want an after-transaction event without asynchronous consumption. And as soon as you're using asynchronous methods, yes, you need to think about transaction semantics. This doesn't look more like an invitation to bugs than being able to call an asynchronous method directly from within a transaction.
I understand that my proposition doesn't fit into the current event model, but the beauty of Spring is that it's open and flexible enough to allow me to use my code without waiting for official Spring support. I just wanted to provide food for thought. If Spring starts considering improvements to its event model, I'd prefer it to go my suggested way, rather than to patch the current poor (IMO) model. |
Stéphane Nicoll commented Annotation-based event listener is on the agenda for 4.2 (see 1bb2a20 and my As for who decides what, I find very disturbing that an event could cause the current transaction to fail; events are processed synchronously by default but you can configure a If we take that out for a second (I guess you'll disagree since that's your current design :-) ), then events can be transactional or we don't really care. There are two says to tackle this. The producer send a "transactional event"Either with a base class or by adding a Of course this should only happen if a transaction is actually running. The receiver request a "transactional event"We could easily flag a receiver to be transactional if we wanted to Putting your "billing record" use case on the table again, this looks like something you could (should) do without the event infrastructure. Thoughts? |
Jean-Baptiste Nizet commented Being forced to extend a Spring class for the event is not ideal, but it's not the most bothering thing. I can live with that. To me, not being able to cause a transaction to rollback from a synchronous event would really be a showstopper, though, and I don't think it's viable anyway. In a typical Spring + JPA/Hibernate application, you want your event listener to be able to access the database, and see its state as it is in the current transaction. You're thus de facto in the current transaction. And if you blindly catch exceptions thrown from the listener method (potentially by JPA/Hibernate) as if nothing happened, you'll be in a situation where the main method continues executing, but with a persistence context/hibernate session that is not usable anyway, since JPA/Hibernate exceptions are not recoverable and leave the context/session in an inconsistent state. Sure, I could implement my billing record use-case without the event infrastructure. But it wouldn't be as elegant, would be tightly coupled, and would introduce hard to remove circular dependencies between modules: the billing module needs to access the operation module to get details about the operation to bill, and the operation module needs to access the billing module to inform it that a new operation must be billed. This is easily and elegantly solved by using synchronous events that participate in the current transaction and allow it to rollback. Again, I'm not inventing a new wheel here. Synchronous events throwing exceptions and causing the transaction to rollback exist in CDI. And not only that: that's the default behavior. Quote from the spec:
And again, being able to process the same event in two different ways, and thus to decide that at the listener side is a real usecase to me. Once again, that's how it's done in CDI and that has proven to be useful and practical. See http://docs.oracle.com/javaee/6/api/javax/enterprise/event/Observes.html. |
Stéphane Nicoll commented I do know how CDI works :) Again, that's just an opinion and I am happy to open the discussion to a wider group. |
Stéphane Nicoll commented Here is a first cut of the implementation (you can look at that branch to check other related commits). It definitely needs some review for corner cases and I want to triple check the potential side effect of using Of course, a first look at the current proposal would be great, please let me know if you find anything suspicious. |
Oliver Drotbohm commented Awesome, Stéphane, I very much like what I see! Great work! Added a few minor comments to the commit but basically it's a very much improved successor of what I originally imagined and should actually also cover the use cases Jean-Baptiste Nizet outlined, right? Although it will only cover the simple after-commit use case, I'll go ahead and update Spring RESTBucks to use that new stuff in a branch and see how this works out. Will keep you posted! |
Stéphane Nicoll commented Thanks Ollie! The code has ben revisited and is available on my branch: Jean-Baptiste Nizet would you mind looking at it as well? |
Jean-Baptiste Nizet commented Sorry for not having taken the time to review this sooner. I won't comment on the internals of the implementation, because you're much more qualified than me on this. But this looks great regarding the end-user API. So, to recap, please correct me if I'm wrong:
If all that is correct, it all looks great to me, except I would find it cleaner if there was an interface, implemented by an injectable bean, that could be used to publish events. I could of course define such an interface and such a bean myself, so that's not a big problem:
|
Stéphane Nicoll commented That's ok. There's no immediate phase for There is an interface to publish an event and it already exists (it was just extended a bit). just inject |
Oliver Drotbohm commented I just created an example for this and wondered why we have |
Stéphane Nicoll commented No reason. Except this: why would you use that? You know the current transaction is going to rollback... |
Oliver Drotbohm commented I saw there's not even a method in |
Oliver Drotbohm opened SPR-12080 and commented
Spring provides an
ApplicationEventPublisher
API that can be used to publish events to other application components. By default, the events are published synchronously using aSimpleApplicationEventMulticaster
when invokingApplicationEventPublisher.publishEvent(…)
directly.In user applications the events publish very often signal the outcome of a business action (e.g. "user created"). Thus it's crucial that these events are only published if the transaction succeeds and thus also after the transactions has concluded.
In this repository I built a proof of concept implementation of an
ApplicationEventMulticaster
that registers aTransactionSynchronization
forApplicationEvents
of a certain type (TransactionBoundApplicationEvent
in my case). This way, the immediate multicasting is delayed to after the transaction commit.This basically works and can be seen in action in the Spring RESTBucks example (necessary configuration, the transaction-bound event and event throwing code). There are a few things that could be changed, added on top or improved:
TransactionAwareApplicationEventMulticaster
has to be configured manually currently. However, it could probably registered automatically if the Spring transaction module is on the classpath. (I can also imagine Boot doing that kind of auto-configuration for now).Issue Links:
Referenced from: commits 4741a12
0 votes, 13 watchers
The text was updated successfully, but these errors were encountered: