apollographql / graphql-subscriptions

:newspaper: A small module that implements GraphQL subscriptions for Node.js
MIT License
1.59k stars 133 forks source link

Changes pushed before the asyncIterator is returned never reach the client #173

Closed theodorDiaconu closed 5 years ago

theodorDiaconu commented 6 years ago

I have a serious problem, how can I make the async iterator push the changes sent to it before the subscription returns example:

subscribe(_, args, ctx) { 
    // create the async iterator
   //  do some other stuff, meanwhile changes get pushed to the async iterator (from some other places)
   // return the asyncIterator
}

Those changes that are pushed before the async iterator is returned never reach the client. Is this by-design, if yes, you know if there is any reason ?

grantwwu commented 5 years ago

Can you explain further what you mean by this? i.e. a runnable test case or a scenario of what you expect to happen.

As far as I know GraphQL subscriptions don't have any sort of "I am guaranteed to get all events published before some time" guarantee.

RedShift1 commented 5 years ago

That's by design and it also makes sense subscribers only get messages after they have subscribed. To make a real-life analogy: if you subscribe to a newspaper you don't get all the newspapers from before the point you were subscribed.

What you could do is keep a buffer somewhere and then use setTimeout() in your subscribe() which pushes the buffer to the pubsub topic.

grantwwu commented 5 years ago

@RedShift1 answered the question I think OP was asking, but I'm not 100% sure.

Fwiw if you need this functionality you can add it as a parameter to your GraphQL subscription, and say you want all messages from before a certain ID/timestamp/etc. This requires support from your PubSubEngine implementation (and support from the backing store, i.e. Kafka/Redis/etc.)

theodorDiaconu commented 5 years ago

@RedShift1 the problem is this: "they subscribed" === the asyncIterator returned (not created)

Ah shit I think I got it now, what if I do it like this:

subscribe(_, args, ctx) {
   return new Promise((resolve, reject) => {
      const iterator = MyFactory.create();
      resolve(iterator);

      // That do the initial pushes
      initialize(iterator);
   });
}

So, the logic here is that, I'm performing initialisations to the asyncIterator only AFTER it has been resolved so the messages added to it get pushed downstream.

@grantwwu I think the issue can be marked as closed.

grantwwu commented 5 years ago

I don't quite get what you are trying to do. I can't see how you can get any guarantees around when the asyncIterator is being returned vs when the asyncIterator is created...

theodorDiaconu commented 5 years ago

@grantwwu let's take this scenario, imagine I use subscription alone, and when I subscribe I want the subscription itself to fill-in the necessary data. I have 3 types of events "added", "changed" and "removed". The problem I had was when I instantiated the asyncIterator and then immediately pushed the 'added', they never reached the client, because they were pushed before subscribe() returned the asyncIterator.

But if I resolve the asyncIterator as a promise and then push the 'added' events, it can work.

grantwwu commented 5 years ago

Oh, you are pushing events to the backing pubsub store upon getting a subscription request.

In general, this can work, although it depends on the semantics of the backing pubsub store. The in memory EventEmitter implementation obviously will work fine, but PubSub systems in general may or may not provide the ability to make these guarantees (i.e. a way of acknowledging published messages such that all clients (or the same client that is servicing the subscription) that ask for messages afterwards get that message).

Anyways, closing this since it seems to be resolved.

theodorDiaconu commented 5 years ago

@grantwwu yes, I'm using PubSub asyncIterator as a means of communicating these changes, the rest is implemented in a custom fashion using a custom store.