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

Support multiplexing with graphql-ws subprotocol #3569

Closed
MohamedSabthar opened this issue Oct 28, 2022 · 2 comments · Fixed by ballerina-platform/module-ballerina-graphql#1081
Assignees
Labels
module/graphql Issues related to Ballerina GraphQL module Points/9 Team/PCM Protocol connector packages related issues Type/Improvement
Milestone

Comments

@MohamedSabthar
Copy link
Member

MohamedSabthar commented Oct 28, 2022

Description:
Multiplexing is not supported by the current graphql subscription implementation when utilizing the graphql-ws/graphql-transport-ws subprotocol.

To handle the multiplexing properly, the incoming messages to the onTextMessage handler must be processed concurrently.
Regarding the current behaviour of the onTextMessage handler, this handler is not dispatched concurrently for received text messages, rather it gets dispatched frame by frame while waiting for the completion of the preceding onTextMessage call. To handle the text message concurrently, we may add a worker inside the onTextMessage handler.

By making this change, the handler will be able to get the complete message even when messages are in-flight to the client.
Once the server has received this complete message from the client, it can stop processing the stream. If we don't stop processing the stream after receiving the complete message, the server will max out with the load. (Current implementation doesn't handle this scenario)

Note: Adding a worker inside the onTextMessage function would necessitate converting every Node class in the graphql package into an isolated class, which might result in performance degradation.

Related Issues (optional):

Suggested Labels (optional):

Suggested Assignees (optional):

@MohamedSabthar MohamedSabthar added Type/Improvement module/graphql Issues related to Ballerina GraphQL module Team/PCM Protocol connector packages related issues labels Oct 28, 2022
@ThisaruGuruge
Copy link
Member

We might be able to do this by making all the parser nodes as readonly classes. Since there is another requirement to do so, I think we should go for it. This needs some changes in the parser as well. Let's break down this task. First, let's try to make the parser nodes readonly by creating them once we have all the information. Then we can proceed with the next step, which is to use a worker inside the onTextMessage() method.

@MohamedSabthar
Copy link
Member Author

MohamedSabthar commented Nov 14, 2022

Sub tasks

  • Make all document tree nodes readonly
  • Update the parser implementation to generate an immutable tree
  • Refactor the visitor implementations which mutates the nodes of the tree.
    - Visitors should create a new readonly node and store them on a map (which holds the modified nodes)
    - When visiting the node visitors should visit the modified node ( if a node modified by a previous visitor)
  • Re-build the document tree with the modified nodes
  • Implement the multiplexing support using worker
  • Load test

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module/graphql Issues related to Ballerina GraphQL module Points/9 Team/PCM Protocol connector packages related issues Type/Improvement
Projects
Archived in project
Status: Standard Lib - PCM
Development

Successfully merging a pull request may close this issue.

2 participants