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

feat: add sync mode for subscription events #114

Merged
merged 1 commit into from
Dec 3, 2023

Conversation

hgiasac
Copy link

@hgiasac hgiasac commented Nov 25, 2023

close #113

Add a new syncMode option to allow subscriptions to execute messages in sequence (without goroutine)

client.WithSyncMode(true)

Copy link

@andrexus andrexus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to have the "sync mode" as the default because, in my opinion, the GraphQL protocol heavily relies on the sequence of messages. I've spent a couple of hours debugging the root cause of the problem, and I think some people may not read the entire documentation but rather create a new issue.
With an extra option, users could enable the "async mode" for improved performance.
As I said, this is just my opinion, and you can decide what defaults the library should have. I am also quite happy with your current solution. Thank you.

@hgiasac
Copy link
Author

hgiasac commented Nov 28, 2023

I haven't tested in-depth about extremely high race condition use cases. However, there would be some locks on the read-message loop. For example, if the execution is too long, other messages will be locked or discarded from the WebSocket client.
So we should keep the async mode to avoid breaking changes for other users. It would be great if you could test and prove the sync mode works without issue. Thanks

@andrexus
Copy link

I have heavily tested this version in my project, which has numerous subscriptions, and I have not encountered any issues. I think it can be merged.

@hgiasac hgiasac merged commit dacf52d into master Dec 3, 2023
2 checks passed
@hgiasac hgiasac deleted the feat/subscription-with-sync-mode branch December 3, 2023 02:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GraphQL subscription sequence is not guaranteed
2 participants