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

fix subscription example in documentation #2677

Merged
merged 1 commit into from
Jun 18, 2023
Merged

fix subscription example in documentation #2677

merged 1 commit into from
Jun 18, 2023

Conversation

leangaurav
Copy link
Contributor

@leangaurav leangaurav commented Jun 16, 2023

Describe your PR and link to any relevant issues.

This PR is to update the documentation on how to implement subscription resolvers. Doc is slightly ambiguous.
Referring to this doc: https://github.com/99designs/gqlgen/blob/master/docs/content/recipes/subscriptions.md

When writing to channel in a select case,
case ch <- t:

the write can block if the consumer is not reading. Can happen during high loads or even randomly.
In such case, the go routine goes to default and exits for no reason.
It should exit when context is cancelled/subscription is closed.

Also documentation is a bit misleading about .

			// The channel may have gotten closed due to the client disconnecting.
			// To not have our Goroutine block or panic, we do the send in a select block.
			// This will jump to the default case if the channel is closed
  1. If writing to a closed channel always panics. Hence won't go to default block. If channel is really closed. Check this example https://go.dev/play/p/yRGO2uYh28M
  2. I think ownership of the channel is with the producer (resolver in this case). So no one else closes channel and no panic happens.

I stumbled upon this when my PR got reviewed and I was asked to write the resolver like how it's written in the example. Writing with with case <-ctx.Done() works fine in our system.
Using default case really doesn't do much harm here, since usually clients consuming subscriptions have a retry. Hence the problem also goes unnoticed.

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

@leangaurav
Copy link
Contributor Author

Also found an open issue regarding same
#2618

@coveralls
Copy link

Coverage Status

coverage: 75.559% (-3.6%) from 79.16% when pulling b7ff9ef on leangaurav:master into d508082 on 99designs:master.

@StevenACoffman
Copy link
Collaborator

Thanks very much! I really appreciate you clarifying the documentation, especially for subscriptions, which have needed some attention for a while. Even our current subscription tests tend to be slow and flaky, which is why I've got the CI set to retry them when they fail.

Thanks again, and I'm looking forward to your next PR!

@StevenACoffman StevenACoffman merged commit 44376e5 into 99designs:master Jun 18, 2023
@leangaurav
Copy link
Contributor Author

HI @StevenACoffman can you point me to the flaky subscription tests. I can take a look and try to fix them.
Also the retry in CI is configured in github actions or somewhere else ?

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.

3 participants