StarpTech / apollo-datasource-http

Optimized JSON HTTP Data Source for Apollo Server
MIT License
73 stars 32 forks source link

[Discussion] How to dedupe concurrent memoizable requests? #34

Open llc1123 opened 2 years ago

llc1123 commented 2 years ago

The Problem

Concurrent requests usually happen:

Multiple requests occur and these requests should be deduped if marked as memoizable.

Examples

const resolvers: Resolvers = {
  UserInfo: {
    name: async ({ mid }, _, { dataSources }) =>
      (await dataSources.example.getUserInfo(mid)).name,
    gender: async ({ mid }, _, { dataSources }) =>
      (await dataSources.example.getUserInfo(mid)).gender,
    avatar: async ({ mid }, _, { dataSources }) =>
      (await dataSources.example.getUserInfo(mid)).avatar,
  },
}
query Users {
  currentUser {
    mid
    name
    avatar
  }
  userInfo(mid: 123) {
    name
    avatar
  }
}

My suggestion

We should use a lazy loader (like graphql/dataloader but much simpler as we don't need batching and caching).

See #29 for my rough workaround.

Minor question

How to define response.memoize if the response is from deduped result?

bradleymorris commented 2 years ago

Why is it not possible to just memoize the original promise? It seems what needs to be done is that the response Promise needs to be created and inserted into the memoization cache Prior to request execution.

That way, any subsequent queries checking the memo cache will fetch the promise and wait for the original execution to complete (and then likewise 'share' the response).

llc1123 commented 2 years ago

I don't think storing the promise in cache is a good idea. How do you deal with promise rejection?

@StarpTech What do you think?

bradleymorris commented 2 years ago

I don't think I see the problem you're alluding to in regards to promise rejection. The memoization store is different from the 'proper' cache and nothing is serialized or written to disk with the dedicated alloc/quicklru memoization cache, as far as I am aware.

So if the promise is rejected, any executions awaiting the promise will be notified as they typically would be, even if they are late arrivers and await promise fulfillment after the promise was either fulfilled or rejected.

I could be over-simplifying something, here, but I think that the Apollo out-of-box REST data source might do something similar to avoid this problem as well.

llc1123 commented 2 years ago

I have submitted a PR #37 memoizing promises and it looks good.

bradleymorris commented 2 years ago

@llc1123 Thank you for your time and effort on this. Hopefully it gets merged and released soon!