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

Fix for missing pre-binding events #156

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

EnderErol
Copy link

@EnderErol EnderErol commented May 17, 2022

This PR resolves an issue related to the order of the bind operations that, in some cases, causes missing events due to the producer producing the events before the binding happens.

The idea is to accumulate the events until all the connections are bound and the subscriptions happen.

On the one hand, the MviCore library does not know about the Observable until we call the bind method. A new component called AccumulatorSubject helps to accumulate the events produced by an Observable until told to stop doing so and drains all the accumulated events. In this case, until all the Connections are bound. On the other hand, a UnicastSubject inside the Binding also collects the events once the Consumer subscribes to Observable.

The reason behind adding UnicastSubject in the middle is that there might exist bounded but not subscribed connections. So if all the bindings are bound, we can drain them but keep the not-consumed events in UnicastSubjects.

Because the UnicastSubject accepts only a single Observer, and the Binder class is reusable for the same Lifecycle object (we can call the start and stop operations multiple times), it creates a new instance for each binding every time we invoke the start method of the Lifecycle.

Also, due to the usage of multiple instances of Binder for subscribing to different Lifecycle events, we keep track of all the Binder instances. We may use the same Observables as the source in distinct Binder instances. So, once we bind all the connections from all the Binders, we can drain the AccumulatorSubjects.

This is not the final solution, just an ideation around a possible one

with(Binding(connection, middleware)) {
bindings.add(this)
if (isActive) {
accumulate()

Choose a reason for hiding this comment

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

What do you think of having accumulate return the subject so that it can be passed to subscribeWithLifecycle without needing a cast?

Copy link
Author

Choose a reason for hiding this comment

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

Passing the subject to the subscribeWithLifecycle method actually would break the solution. Apart from the above line, we call it from the bindConnections function, which ensures that all the accumulations start before the bindings happen.

On the other hand, we could avoid the binding by adding type variables everywhere. I think it could significantly complicate the solution. WDYT about discussing it and maybe fixing it in another PR?

@zsoltk
Copy link
Contributor

zsoltk commented Jun 27, 2022

It'd be great to see some test scenarios for expectations covering bind-unbind-rebind cycles.

@EnderErol EnderErol marked this pull request as draft October 5, 2023 10:18
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.

None yet

4 participants