urql-graphql / urql

The highly customizable and versatile GraphQL client with which you add on features like normalized caching as you grow.
https://urql.dev/goto/docs
MIT License
8.54k stars 444 forks source link

Graphcache: mutations with different files in parallel, results lost #3541

Closed ursarik closed 1 month ago

ursarik commented 3 months ago

Describe the bug

Results get lost if you upload multiple files in parallel and use Graphcache. Seems like it happens because of stringify in variables.ts in core package If you will add

if (x instance File) {
  const key = cache.get(x) || Math.random().toString(36).slice(2);
  cache.set(x, key);
  return stringify({ __key: key });
}

everything works fine

Reproduction

https://codesandbox.io/p/devbox/mystifying-borg-g7zgym?file=%2Fsrc%2FFileUpload.jsx%3A22%2C1

Urql version

urql v4.0.6, urql/exchange-graphcache v6.5.0

Validations

JoviDeCroock commented 3 months ago

We used to do that before https://github.com/urql-graphql/urql/pull/3169 however that didn't work out. I guess the issue is that we are using operation.key in GraphCache for mutations while we decided a while ago to use a different type of _identity for that.

I guess this could be the cause for missing a few results with parallel identical (observably identical) mutations

kitten commented 3 months ago

@JoviDeCroock: The layering of mutations is still relevant, but if results conflict, they can override each other.

@ursarik: This really depends on how you use the result, typically; Each duplicate mutation gets the same key, unless the variables change it, but still flows through the operation chain separately.

This doesn't really matter until Graphcache writes results to its cache in layers, which may then be re-used across multiple mutations with the same key.

Meaning, we might need you to be more specific than:

Results get lost

Especially since it's unclear what the reproduction is trying to reproduce and errors out with a 502 error.

jongbelegen commented 1 month ago

I'm having a similar issue I think.

I like to use my client directly without a hook if I do not need the result state to be in the react tree.

I have a function that gets called in parallel to retrieve an upload URL from the backend:

mutation GetUploadUrl {
    getUploadUrl {
        path
        url
        __typename
    }
}
allUploads.map(getUploadUrl)

async function getUploadUrl() {
  console.log("before getUploadUrl");
  const { error, data } = await client
    .mutation(GetUploadUrlDocument, {})
    .toPromise();
  console.log("after getUploadUrl");

I notice that only the last result resolves the promise.

I can bypass it for now by just passing something random unique variable:

    .mutation(GetUploadUrlDocument, {
      randomId: getUniqueId(),
    })

But I'm not sure if a never-resolved promise is ever what you want. Doesn't this introduce possible memory leaks for example? I also have a lot of polling code that waits for the request to be finished where this behavior would also never wanted.

ursarik commented 1 month ago

@kitten

Especially since it's unclear what the reproduction is trying to reproduce and errors out with a 502 error.

There is console.log("response:", { data, error, file }); in FileUpload.jsx on line 23.

To reproduce the problem, select 2 or more files and press Upload. You will get only one log in the dev tools, even though all requests have completed. So, obviously, all responses except the first one got lost.

JoviDeCroock commented 1 month ago

@ursarik that does not reproduce for me, see attached screenshot image

ursarik commented 1 month ago

@JoviDeCroock looks strange that's what I get. see attached screenshot

image

UPD: I guess I figured out what happens in your screenshot. Dunno how that happened simultaneously, but you selected two files twice, then pressed Upload twice, and then you got only 1 response from each selection(2 in total), but you should get 4 responses in total (2 responses from each upload).