Cylc-flow: GraphQL subscriptions on the same request/connection share the same ID

[dwsutherland]

An issue in master but manifests itself in cylc/cylc-ui#543 :
https://github.com/cylc/cylc-ui/pull/543#issuecomment-852549778

UI subscriptions share the same subscription/graphql handler, where each GraphiQL request creates a new handler at the UIS.. Which causes clashes on resolving data.

Steps to reproduce the bug
Using cylc/cylc-ui#543 branch:

• run the UI
• cylc install workflow
• cylc clean workflow
• Repeat above two steps until after an install, the workflow doesn't appear in the UI.

Expected behavior
UI would have a clean feed from each subscription; multiple subscriptions wouldn't interfere with each other.

Additional context
To show this is the issue, I put an ID;

            'subscription_id': uuid4(),

in the handlers context:

class SubscriptionHandler(BaseHandler, websocket.WebSocketHandler):
    def initialize(self, sub_server, resolvers):
        self.queue = Queue(100)
        self.subscription_server = sub_server
        self.resolvers = resolvers
    def select_subprotocol(self, subprotocols):
        return GRAPHQL_WS
    @websockets_authenticated
    @authorised
    def get(self, *args, **kwargs):
        # forward this call so we can authenticate/authorise it
        return websocket.WebSocketHandler.get(self, *args, **kwargs)
    @websockets_authenticated
    @authorised
    def open(self, *args, **kwargs):
        IOLoop.current().spawn_callback(self.subscription_server.handle, self,
                                        self.context)
    async def on_message(self, message):
        await self.queue.put(message)
    async def recv(self):
        return await self.queue.get()
    def recv_nowait(self):
        return self.queue.get_nowait()
    @property
    def context(self):
        wider_context = {
            'request': self.request,
            'resolvers': self.resolvers,
            'subscription_id': uuid4(),
        }
        return wider_context

And when I started up the UI (with it's two subscriptions), along with the two GraphiQL subscriptions:

.
.
.
[I 2021-06-04T17:30:40.956 JupyterHub log:189] 200 GET /hub/api/authorizations/token/[secret] ([email protected]) 43.76ms
17:30:42.218 [ConfigProxy] error: 503 GET /user/sutherlander/favicon.png socket hang up
[I 2021-06-04T17:30:42.339 JupyterHub log:189] 200 GET /hub/error/503?url=%2Fuser%2Fsutherlander%2Ffavicon.png (@127.0.0.1) 118.23ms
d9d09ba3-ef7c-4479-ba2d-4cf0cacd6d9a
d9d09ba3-ef7c-4479-ba2d-4cf0cacd6d9a
15cbbf57-c8b5-4104-8155-b8228a4e5b1c
2eb00ab2-2494-4700-b442-8ace6d0fe6cc

from a print in in the root resolver (cylc.flow.network.resolver):

        if 'subscription_id' in info.context:
            print(info.context['subscription_id'])

You can see the first two are the same (the UI dashboard and gscan subscriptions share the same request)

GraphQL subscriptions need to use an object local to only it's resolvers to store the sub_id (not use context which is shared with all subscriptions in the request)

Pull requests welcome!
This is an Open Source project - please consider contributing a bug fix
yourself (please read CONTRIBUTING.md before starting any work though).

[kinow]

@dwsutherland I am confused at how we are going to solve the issue. In the client, we have 1 HTTP request, that is upgraded to a WebSockets connection.

Then in the same connection we start the two subscriptions, each with a different ID.

I think this is valid per GraphQL specs. And this is all done internally in ApolloClient (i.e. I am creating subscriptions, apollo is doing the rest).

Are you suggesting we start 2 websockets connections? One for each sub?

[dwsutherland]

We just need some sort of unique identifier for each subscription, that we can pass to all the downstream resolvers to give them "context".

[kinow]

We just need some sort of unique identifier for each subscription, that we can pass to all the downstream resolvers to give them "context".

We should have one per connection/request. The client provides an operation ID, which can be used to identify the subscription. Would that work?

[dwsutherland]

We should have one per connection/request. The client provides an operation ID, which can be used to identify the subscription. Would that work?

But we have multiple subscriptions on the connection/request.. But we need an object passed to the resolvers of each subscription that is unique to that subscriptions.

We may already have it in the info arg that's passed to all resolvers (i.e. get_nodes_all(root, info, **args)), it has some common references like info.context but info itself references some unique to subscription/query stuff.

[kinow]

But we have multiple subscriptions on the connection/request..

Correct. The client will have initiated, probably, two subscriptions on 1 connection/request. Subscription ID=1 being the GScan subscription, and ID=2 the Dashboard or the Tree View subscription.

I was thinking that you could use this ID=??? to identify the subscription in the UIS.

[dwsutherland]

I was thinking that you could use this ID=??? to identify the subscription in the UIS.

Yes, that would be great! where is it? in the request object? (obviously 1 & 2 are too simple, but we can at least work from there)

[kinow]

Each WS message contains the ID.

That ID should be available in the server side when the subscription is created. I believe it is the op_id in our tornado-graphql code: https://github.com/cylc/cylc-uiserver/blob/a6c5aad2dd9df7533efc2b95a52ae737dbfdff2f/cylc/uiserver/websockets/tornado.py#L129

Maybe we just need to add it to the context, or pass it to some function so that it reaches the resolvers and rest of graphql-ws/graphql code?

[dwsutherland]

Maybe we just need to add it to the context, or pass it to some function so that it reaches the resolvers and rest of graphql-ws/graphql code?

Yeah. I should explore the info arg a bit more .. bound to be something there (and possibly this too).

[dwsutherland]

Actually info isn't shared, but info.context is shared between subscriptions... So fix incoming, I'll just use info.variable_values or something.

(I don't think it would matter about the op_id as if you had multiples, the resolver wouldn't know which one it is unless it parsed the graphql doc and compared)

[kinow]

Great @dwsutherland ! I think we should transfer this issue over to cylc/cylc-uiserver too?

[dwsutherland]

Great @dwsutherland ! I think we should transfer this issue over to cylc/cylc-uiserver too?

The fix is actually at cylc/cylc-flow

[kinow]

Thanks @dwsutherland !!!