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

cache.writeFragment and commutative updates #633

Closed
zenios opened this issue Mar 17, 2020 · 16 comments · Fixed by #638
Closed

cache.writeFragment and commutative updates #633

zenios opened this issue Mar 17, 2020 · 16 comments · Fixed by #638
Labels
bug 🐛 Oh no! A bug or unintented behaviour.

Comments

@zenios
Copy link

zenios commented Mar 17, 2020

After upgrading from graphcache 2.2.2 problems with writeFragment and commutative updates started appearing again.

The behaviour is exactly the same as #574 but not so often.

I think it has something to do with the change on 2.2.3 to include subscription also

What the component does is at a click of a button a mutation is executed and two responses arrive

  • One from the mutation result which updates an entity using writeFragment
  • And another from a subscription which also updates another entity using writeFragment

Some background

  • It was working fine with 2.2.2
  • I tried all versions after 2.2.2
@kitten
Copy link
Member

kitten commented Mar 17, 2020

All changes now take precedence over subscriptions, which is our intended way of tackling commutativity; our assumption is that it’s unnecessary to handle subscriptions (or rather to deliver events to a client) where a mutation has made the modification.

So as long as both updates result in the same cache changes, you should be fine.

I’d be interested though in the exact case you’re running into so we can reevaluate our assumptions 👍

@zenios
Copy link
Author

zenios commented Mar 17, 2020 via email

@kitten
Copy link
Member

kitten commented Mar 17, 2020

Ok, we may need some details here, I just double checked your description and you do indeed say "updates an entity" and "updates another entity", so we may need some usage details, because I don't think we made any changes that would impact two completely unrelated entities to not update when they're created / written to at the same time.

@kitten kitten added the needs more info ✋ A question or report that needs more info to be addressable label Mar 17, 2020
@zenios
Copy link
Author

zenios commented Mar 17, 2020 via email

@kitten
Copy link
Member

kitten commented Mar 17, 2020

So a minor reproduction would be useful, but I don't believe it's the same issue as we're testing for that now.

Anyway, we had a hunch that it may be related to garbage collection. Could it be that you're writing to a fragment that is "dangling", meaning that there's no path from Query to the fragment inbetween your results?

@zenios
Copy link
Author

zenios commented Mar 18, 2020

If it was than then i assume it would happen every time.

It happens at a rate of 1 per 10

@zenios
Copy link
Author

zenios commented Mar 18, 2020

I have a reproduction

https://codesandbox.io/s/confident-surf-qu4nb

Instructions :)

  1. Open the same sandbox in two tabs
  2. On the first tab select from 0 to 1
  3. On the second tab select from 1 to 0
  4. Press send button of the first tab You will see both users 0 balance and users 1 balance to update. You will also see the subscription data arriving. Changes will be reflected on both tabs
  5. Press send button on the second tab. Changes are only applied on the second tab even though the subscription data arrived on the first tab also

@zenios
Copy link
Author

zenios commented Mar 18, 2020

No its not the same changes.subscription updates one entity mutation result updates another entity

On Tue, 17 Mar 2020, 23:09 Phil Plückthun, @.***> wrote: All changes now take precedence over subscriptions, which is our intended way of tackling commutativity; our assumption is that it’s unnecessary to handle subscriptions (or rather to deliver events to a client) where a mutation has made the modification. So as long as both updates result in the same cache changes, you should be fine. I’d be interested though in the exact case you’re running into so we can reevaluate our assumptions — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#633 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAV3YMI2TGDGG2PFTB7PGLRH7RILANCNFSM4LNYGR6A .

FYI It turns out that mutation updates both entities.

One that will also be updated from the subscription and the other which will be updated only through the mutation

So just to be clear

On the click of a button a mutation happens

  • Subscription arrives that will updateFragment of Entity A
  • Mutation result arrives that will updateFragment of Entity A and Entity B
  • updateFragment of both subscription and mutation for Entity A will contain the same data.

@JoviDeCroock
Copy link
Collaborator

JoviDeCroock commented Mar 18, 2020

Hey,

I've been investigating your issue and there seems to be an issue in your application logic, both balances change but only one is sent back to the other tab.

However when this is the result of my operation on tab 1:

mutation {__typename: "User", id: "1", balance: 1}
mutation {__typename: "User", id: "0", balance: 0}

Then I see this subscription result on my second tab:

subscription {__typename: "User", id: "0", balance: -1}

Don't get me wrong it's entirely possible there is an urql issue present but the erroneous responses from the API make it very hard to find.

EDIT: wait I must be overlooking something, let me get back on this.

@zenios
Copy link
Author

zenios commented Mar 18, 2020 via email

@zenios
Copy link
Author

zenios commented Mar 18, 2020 via email

@kitten kitten added bug 🐛 Oh no! A bug or unintented behaviour. and removed needs more info ✋ A question or report that needs more info to be addressable labels Mar 18, 2020
@kitten
Copy link
Member

kitten commented Mar 18, 2020

We've got an idea what's going on and are actively working on a fix. Seems to be a problem with timing when results come in repeatedly, especially for subscriptions, which also get their state erased in some cases.

@zenios
Copy link
Author

zenios commented Mar 18, 2020

repeatedly you mean one mutation and one subcription like in the sandbox?

@kitten
Copy link
Member

kitten commented Mar 18, 2020

@zenios There's more details here: cb0e557

So it's hard to describe and figure out which edge-case the sandbox hits exactly... But we're trying to write some test cases. We're hitting impossible conditions here due to some small flaws in the order of layer calls, so it's hard to say which edge case (that were all supposed to not happen) you're hitting.

@JoviDeCroock has copied over your sandbox to the CodeSandbox packaged sandbox with the fixes from #638: https://codesandbox.io/s/recursing-pine-pvysc
And it seems to not suffer from these bugs anymore (?)

@kitten
Copy link
Member

kitten commented Mar 18, 2020

@zenios we’ve added some more tests and I’m quite sure we’ve fixed the issue(s). Sorry for making your life a little harder! I hope we won’t encounter more of these bugs.

But on the flip side, they really pave the way for eventually adding offline support! Thanks for having an eye on the library and testing out new versions! ♥️🙌

@zenios
Copy link
Author

zenios commented Mar 18, 2020

No worries and thanks for your support. Urql is becoming the defacto graphql library

Will wait for the next release in order to test it and let you know.

Thanks again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Oh no! A bug or unintented behaviour.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants