GQL_STOP sent after "complete" from server

[leebenson]

It looks like a GQL_STOP message is being sent from the client, after receiving a completion message back from the server, resulting in the server then returning an error since the subscription is no longer active.

My expectation would be that if the server has initiated a complete message, the client should silently unsubscribe.

FWIW, I'm using gqlgen in Go for the server.

[gklijs]

According to the spec, for subscriptions, complete should only be send from the server, after the cleint send a stop..

[leebenson]

According to the GraphQL June 2018 spec (emphasis mine):

Event Streams

An event stream represents a sequence of discrete events over time which can be observed. As an example, a “Pub‐Sub” system may produce an event stream when “subscribing to a topic”, with an event occurring on that event stream for each “publish” to that topic. Event streams may produce an infinite sequence of events or may complete at any point. Event streams may complete in response to an error or simply because no more events will occur. An observer may at any point decide to stop observing an event stream by cancelling it, after which it must receive no more events from that event stream.

In any case, it's moot/an error for Apollo to send a GQL_STOP after receiving a 'complete' back from the server, since there's nothing to be stopped.

[gklijs]

Your are correct. https://github.com/apollographql/subscriptions-transport-ws/blob/master/PROTOCOL.md needs an update trough. I actually have a use case where it's just one message from a subscription, where it would be nice if the server could close that subscription. But we need a spec for it. With a new spec appropriate changes can be made.

[leebenson]

I'm not actually sure we need to wait on a spec change, personally. The 2018 spec already makes it clear that subscriptions can complete at any point, in response to an error or no further events. It leaves it open as to whether this should be from the client or server.

The Apollo subscription protocol doc simply states:

GQL_COMPLETE
Server sends this message to indicate that a GraphQL operation is done, and no more data will arrive for the specific operation.

It mentions a client closing the subscription, but AFAICT, doesn't explicitly rule out the complete message being sent first.

All that's required to fix this issue is for for Apollo client to avoid sending a GQL_STOP after receiving a GQL_COMPLETE. This behavior makes no sense in any context, since a client initiated event would receive the 'complete' as its final message and wouldn't follow-up. My suggestion is that server-closed events are treated the same way, which I believe is in accordance with the existing spec (or only a slight Apollo-level spec modification, if there's perceived ambiguity.)

FWIW, the general implementation I've seen for most GraphQL server libs that implement WebSockets for subscriptions, is to send a 'complete' when the socket is closed explicitly on the server-side. I think it'd be simple and sensible change just to assume in the Apollo client this means the client should unsubscribe with no further messages.

[gklijs]

It depends, some send the 'complete' only after the client called 'stop'. But for this issue indeed we don't need a changed spec. A new spec should probably also allow 'uploads' and partial queries.

[leebenson]

It depends, some send the 'complete' only after the client called 'stop'.

I'm outside of context discussing other libs in this issue, but FWIW, I think it's a poor decision for a server lib to only send 'complete' after a client decides to stop. There are plenty of scenarios where the server can discern that it's reached the end of a dataset, has encountered an irrecoverable error, or simply may choose to close the WebSocket.

Not informing the client of this means either wasting resources keeping a redundant socket open, or worse, closing it without informing the client that was the intention, which could then spur Apollo to attempt an automatic reconnect. IMO, a server should always send a 'complete'. That's at least the case for gqlgen and at least a couple of other libs I've used. Not sure how Apollo server handles it - might be worth testing.

[gklijs]

I've mainly used Jvm servers. It's not really a redundant socket, as all subscriptions use the same websocket. But you totally right something might happened either an error, or just how it's implemented ending the subscription. For these to work properly between clients and servers we need a proper spec through. For example it might be nice in the 'stop' message to have something like a reason. Also other edge cases, like subscribing with an id for a subscription that's still running.

[leebenson]

For example it might be nice in the 'stop' message to have something like a reason. Also other edge cases, like subscribing with an id for a subscription that's still running.

👍 I'd definitely welcome any additional meta - and being able to resubscribe to a 'live' subscription.

In the meantime, if the redundant GQL_STOP post-complete could be halted, that would at least save a round-trip to the server.

It's not causing any major side-effect at this point, since the Apollo client seems to simply ignore it (and gqlgen just responds that the subscription ID asked to be stopped isn't valid), but it's extra noise and bandwidth all the same.

[onhate]

I see the same happening on my custom server implementation, if I don't send GQL_COMPLETE the client does not calls the Promise.then method which makes me obligated to send GQL_COMPLETE after queries and mutations data is send, and if I send right after the client receives it sends a GQL_STOP that I also reply with GQL_COMPLETE. As I am running serverless that generates an extra lambda invocation for each query/mutation which is (in my point of view) unnecessary and avoidable.

https://github.com/apollographql/subscriptions-transport-ws/blob/master/PROTOCOL.md

Server sends GQL_COMPLETE if the operation is a query or mutation (for subscriptions, this sent when unsubscribing)

[onhate]

I've raised a PR that fixes this scenario: #775

[onhate]

@leebenson @gklijs please help me promote the PR so we can have this solved, thanks.