feat: add subscription mechanism to receive all processed events #21402
Conversation
signals/src/main/java/com/vaadin/signals/impl/AsynchronousSignalTree.java
Outdated
Show resolved
Hide resolved
…al-events' into taefi/enable-subscribing-to-signal-events
signals/src/main/java/com/vaadin/signals/impl/SynchronousSignalTree.java
Outdated
Show resolved
Hide resolved
signals/src/test/java/com/vaadin/signals/impl/AsynchronousSignalTreeTest.java
Outdated
Show resolved
Hide resolved
signals/src/test/java/com/vaadin/signals/impl/AsynchronousSignalTreeTest.java
Outdated
Show resolved
Hide resolved
signals/src/test/java/com/vaadin/signals/impl/AsynchronousSignalTreeTest.java
Outdated
Show resolved
Hide resolved
signals/src/test/java/com/vaadin/signals/impl/SynchronousSignalTreeTest.java
Outdated
Show resolved
Hide resolved
signals/src/main/java/com/vaadin/signals/impl/AsynchronousSignalTree.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| public void markAsAborted() { | ||
| changes.notifyResultHandlers(CommandResult.rejectAll(results, | ||
| "Transaction aborted")); | ||
|
|
||
| for (SignalCommand command : changes.getCommands()) { |
There was a problem hiding this comment.
I don't think this is correct. Commands for an aborted transaction will not be "processed" in the sense that that they will not be involved in the official event ordering. An otherwise valid command that is aborted would otherwise be interpreted as accepted by other observers. This should not be an issue for the Hilla integration since I assume it has not case that would lead to aborting a transaction.
There was a problem hiding this comment.
Commands for an aborted transaction will not be "processed" in the sense that that they will not be involved in the official event ordering.
Well, it depends on what we interpret the meaning of processed. To me, the term "processed" means a command that is delivered to the signal(tree), and either managed to change the value or got rejected. The subscriber is just interested in an indicator of signal(tree) received and considered this command (regardless of the result).
Or maybe, I'm not understanding the question here.
There was a problem hiding this comment.
The subscriber is supposed to see all submitted commands in the order they have been processed. The commands from an aborted transaction are never really submitted from the tree's point of view, and should thus never be delivered to subscribers. Preventing commands from being submitted is the whole point of aborting a transaction in this way.
If we would show aborted commands to the listener, then we again need to introduce some external way for the subscriber to know that those commands were actually not accepted by the tree since it's not apparent from the previous state of the tree that those commands wouldn't be accepted. I don't see any reason for ever showing those aborted commands to anyone else so I don't think we need to have that as a reason to also start passing the status.
There was a problem hiding this comment.
I don't see any reason for ever showing those aborted commands to anyone else
Just to make sure we are on the same page: by "anyone else" did you mean:
- that only the sender of an aborted command should get it back?
- This looks ideal, and should technically be possible to create a mapping in Hilla's server-side handler to prevent sending the failed ones to others.
- Or, no one should get it back?
- I don't agree. From a Hilla client's POV (an external observer), it only cares about making sure that my command "arrived" to the server, server look at it, and whatever was the decision (acceptance or rejection) its effect is already reflected in the local server's confirmed state. So it should be able to either apply or reject it on its own server's confirmed state based on the guaranteed order of received commands and the previous state.
we again need to introduce some external way for the subscriber to know that those commands were actually not accepted by the tree since it's not apparent from the previous state of the tree
Maybe, I'm missing all the reasons for an aborted transaction, and there are commands that should not be even delivered back to the client that issued it. If the sender of an aborted/failed command doesn't receive it back, it remains in the "locally applied, not reflected in server's state" forever. Ofc, there could be mechanisms to decide a command wasn't accepted, however, those could be partial.
There was a problem hiding this comment.
A transaction can be aborted only when running inside Signal.runInTransaction. I don't see why the Hilla integration would do that which means that those concerns are not relevant.
There was a problem hiding this comment.
No wait. I forgot that the non-transactional path still goes through the same logic when canCommit() is false. So that complicates things significantly. Maybe we have to go back to passing the status to the subscriber after all.
|
| @@ -90,6 +92,8 @@ public enum Type { | |||
|
|
|||
| private final Type type; | |||
|
|
|||
| private final Set<Consumer<SignalCommand>> subscribers = new HashSet<>(); | |||
There was a problem hiding this comment.
nit: I like my subscriber ordered to ensure consistency
Description
This pull request introduces a new subscription mechanism for signals, enhances utility methods, and adds comprehensive test coverage to ensure the correctness of new functionality. The changes are primarily focused on enabling subscribers to receive notifications about the results of processed commands and improving the maintainability of the codebase.
New Subscription Mechanism for Signals:
subscribeToPublishedmethod inSignalTreethat allows subscribers to listen for the results of processed commands. Subscribers can be removed using the returned callback.Utility Enhancements:
SignalUtilswith methods to retrieve the underlying signal tree and validate commands. This class is marked as internal API.Fixes #21370
Type of change
Checklist
Additional for
Featuretype of change