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

Unsubscribe some times do not send subscriptionId #8

Closed
chengyin opened this issue Nov 21, 2017 · 5 comments
Closed

Unsubscribe some times do not send subscriptionId #8

chengyin opened this issue Nov 21, 2017 · 5 comments

Comments

@chengyin
Copy link

Sorry I haven't created a small reproducible case. Wanted to start the conversation first.

I'm using subscription with apollo. I am trying to unsubscribe with the function returned by apollo's subscribeToMore. I see that call get passed into socket-apollo-link. Inspecting the WS frames I see that no subscriptionId is sent with the majority of the requests:

screen shot 2017-11-20 at 11 26 23 pm

Down below, there is a case where it is sent.

When no subscriptionId is provided it actually cause a server side error.

Part of the code:

export default ({
  subscribeToMore,
  addedSubscription,
  removedSubscription,
}) => postId => {
  const unsubscribes = [
    subscribeToMore({
      document: addedSubscription,
      variables: { postId },
      updateQuery: (prev, { subscriptionData }) => {
        # add to cache
        return {
          ...prev,
          data,
        };
      },
    }),

    subscribeToMore({
      document: removedSubscription,
      variables: { postId },
      updateQuery: (prev, { subscriptionData }) => {
        # remove from cache
        return {
          ...prev,
          data,
        };
      },
    }),
  ];

  return () => {
    unsubscribes.forEach(unsubscribe => {
      unsubscribe();
    });
  };
};
@tlvenn tlvenn added the bug label Nov 21, 2017
@mgtitimoli
Copy link
Member

Hi @chengyin,

Based on what you are saying, I understand that this doesn't happen all the time, but aleatory with some of them, right?

@tlvenn
Copy link
Member

tlvenn commented Nov 21, 2017

@chengyin , Is there any chance you are trying to unsubscribe before having subscribed ?

@chengyin
Copy link
Author

Thanks for the quick responses!

First I do not believe I'm trying to unsubscribe before subscribing. Based on my understanding, the only time that could happen is when the server response is delayed:

  1. subscription request sent to server, hasn't heard back
  2. unsubscribe
  3. server sent subscription confirmation

I don't think that was happening in my case.

Now, from my actual debugging. This line processes the subscription confirmation only if the response event is subscription:data. However, in the response I saw, the event is phx_reply. Here is the response JSON:

{
  "join_ref": "1",
  "ref": "2",
  "topic": "__absinthe__:control",
  "event": "phx_reply",
  "payload": {
    "status": "ok",
    "response": {
      "subscriptionId": "__absinthe__:doc:73031069"
    }
  }
}

Notice subscriptionId was in there. In the screenshot above, I didn't see any subscription:data sent. This should explain why subscriptionId wasn't set on some notifier? However this still don't explain why in the screenshot message 10:11 does have the subscriptionId.

I'm not sure what's the expected message form from absinthe. I searched the whole absinthe org on GitHub for subscription:data, the only place I saw were in absinthe_plug tests. if it helps, our absinthe versions:

     {:absinthe, "1.4.1"},
     {:absinthe_phoenix, "1.4.0"},
     {:absinthe_ecto, "0.1.3"},
     {:absinthe_plug, "1.4.0"},

I think absinthe has 1.4.2. Changelog didn't mention anything related. We have the latest everything else.

@mgtitimoli
Copy link
Member

mgtitimoli commented Nov 21, 2017

That code is OK @chengyin, as it's the one that handles the incoming data.

Subscriptions (as any other request) are sent using pushRequest module, and is in the succeed handler where the notifier is updated with the incoming subscriptionId (here).

What it should be happening (I'm currently working on to remove this requirement), is that currently to cancel a subscription, the latest version of the notifier has to be passed to cancel, since it's the one that has subscriptionId set on it. This one is obtained in the onStart handler of the observer.

This week I will release a new version where you won't need to pass the latest notifier (with subscriptionId set) in order to unsubscribe, it will be enough to pass any version of them.

Meanwhile, you will have to store the notifier given in observer.onStart and use this one to unsubscribe.

@chengyin
Copy link
Author

Thanks. Setting a breakpoint at the line you pointed did show me that subscriptionId was set.

I think saving notifier is non trivial for me since I use the socket-apollo-link, and it doesn't expose those details. I will wait for your fix. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants