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

Modifying sessionManager from an observer may cause observers seeing events in different order and other "in-between states" #400

Open
pocmo opened this issue Jul 10, 2018 · 9 comments

Comments

@pocmo
Copy link
Contributor

commented Jul 10, 2018

For #379 I added code in the sample browser that will add a new tab if all tabs get closed. However this code is indirectly called from a session manager observer. If there are more observers then the following ones would get called with the session manager state already changed - or other calls in between.

Let's say there are two observers (A / B) registered on the session manager. And the session manager has one session X.

  • session manager (X)
  • X is removed from session manager.
  • A.onSessionRemoved(X) gets called: A adds a new session Y to session manager and this will notify observers:
    • A.onSessionAdded(Y) gets called
    • B.onSessionAdded(Y) gets called
  • B.onSessionRemoved(X) gets called.

In the end A and B have observed the same events. But in different order. B never saw an empty session manager:

  • For A: (X) -> () -> (Y)
  • For B: (X) -> (X,Y) -> (X)

@pocmo pocmo added this to the 0.15 🌈 milestone Jul 13, 2018

@pocmo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2018

I wonder what a good solution to that is.

The only solution I can come up with is that all modifying methods are not executed synchronously but on a single-threaded executor. With that modifications during a dispatch would just be queued until the current update is made and dispatched to all observers.

@csadilek

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

I think we run into this because we're using an reentrant lock (synchronized(observers)) in our ObserverRegistry.notifyObservers. So, when notifying one observer that notifies more, those subsequent calls can enter and execute right away (before the original observers are notified.) If we used a semaphore/mutex instead those calls would instead have to wait until all observers are executed. I think with this change we could still execute them synchronously which is simpler than introducing a thread/queue?

@pocmo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2018

If we used a semaphore/mutex instead those calls would instead have to wait until all observers are executed.

Wouldn't we deadlock then? It's the same thread that is notifying observers and then modifying sessionManager?

@csadilek

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

Wouldn't we deadlock then? It's the same thread that is notifying observers and then modifying sessionManager?

Yes, we can't block on the mutex. I was mainly trying to look for a synchronous solution. Here's what I was thinking re: mutex/semaphore (tryAcquire doesn't block):

private val notifyMutex = Semaphore(1)
private val queue = LinkedList<T.() -> Unit>()
...
override fun notifyObservers(block: T.() -> Unit) {
        if (notifyMutex.tryAcquire()) {
            observers.forEach {
                it.block()
            }
            notifyMutex.release()

            if (queue.isNotEmpty()) {
                notifyObservers(queue.pop())
            }
        } else {
            queue.push(block)
        }
    }

Would need to try this. Am I missing something?

@pocmo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 16, 2018

We would need to not only prevent the notification of the observers but also the change of the Session properties. Otherwise some observers will get an already modified session object.

@csadilek

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

So, I was just focusing on making sure observers see consistent event ordering. I am not sure we need to prevent any (session manager) side-effects in observers until they've all run...

@csadilek

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2018

...but then again, it seems the same pattern could work for SessionManager as well. We'd queue up all changes that happen while its notifying its observers, and apply after. Observers could have all kinds of other side-effects though e.g. they could change sessions directly...

@pocmo pocmo self-assigned this Jul 19, 2018

@pocmo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2018

Assigned myself to sketch something.

pocmo added a commit to pocmo/android-components that referenced this issue Jul 19, 2018

@pocmo pocmo modified the milestones: 0.15 🌈, 0.16 🦄 Jul 20, 2018

pocmo added a commit to pocmo/android-components that referenced this issue Jul 25, 2018
pocmo added a commit to pocmo/android-components that referenced this issue Jul 25, 2018

@pocmo pocmo modified the milestones: 0.16 🦄, 0.17 🌊 Jul 25, 2018

@pocmo pocmo added the 🔬 Research label Jul 30, 2018

@pocmo pocmo removed this from the 0.17 🌊 milestone Aug 3, 2018

@pocmo pocmo removed their assignment Jan 3, 2019

@pocmo

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2019

Heh, this issue is still open. Closing since the solution we came up with is re-designing a new component (browser-state). :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.