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 observer/unobserve case #165

Closed
wants to merge 1 commit into from
Closed

Conversation

arskama
Copy link
Contributor

@arskama arskama commented Dec 16, 2022

fixes #164


Preview | Diff

@arskama
Copy link
Contributor Author

arskama commented Dec 16, 2022

Please have a careful look at this. I m not 100% sure of this fix.

@kenchris
Copy link
Contributor

This is not very well defined, it's basically a pseudo instruction.

Behavior might differ depending on when disconnect/unobserve is called. Like after or before it is appended to the active observers list.

@kenchris
Copy link
Contributor

I think the easiest way to fix this is to have some kind of lock (internal promise chain) activated whenever an observe call has not settled, and then only run disconnect and unobserve when the lock is unlocked. Then those would have to become promises, which I guess it probably the only right way to handle this.

Basically when we create a promise in observe() then we add it to an internal list of promises, and we chain disconnect/unobserve on that.

But we need to make sure it works fine with calls like "observe, unobserve, observe" etc. So the unobserve/disconnect calls will need to copy the list of current pending promises

@kenchris
Copy link
Contributor

Maybe this would work:

  • store pending observe promises
  • disconnect: reject all pending observe promises, but make sure that everything has been removed from "active observer list" - that should be fine
  • unobserve: iterate over the pending promises and reject the ones matching the source. Remove from "active observer list" - might have been added, might not

Copy link
Contributor

@kenchris kenchris left a comment

Choose a reason for hiding this comment

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

This won't work according to our discussion

@kenchris kenchris closed this Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants