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

Add subscriptions.md recipe to docs #2346

Merged

Conversation

Unkn0wnCat
Copy link
Contributor

@Unkn0wnCat Unkn0wnCat commented Aug 28, 2022

I've recently wanted to add subscriptions to my GraphQL-API and couldn't find anything in the official docs.

I think that's a shame, so I've decided to change it. 😄

This PR contains docs/content/recipes/subscriptions.md which in turn contains my tutorial on subscriptions.

If you read this, please take some time to proof-read the document and if you find a mistake, please do a code-review with a change suggestion. I've tried proof-reading to the best of my ability, but more eyes are always better!

I have:

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

Closing #953

@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 75.479% when pulling 6f798a7 on Unkn0wnCat:docs-recipe-subscriptions into 2ba8040 on 99designs:master.

@StevenACoffman
Copy link
Collaborator

StevenACoffman commented Sep 2, 2022

This looks great. Sorry for the delay! I was trying to figure out some way to scrape this and other tutorials to test that it continues to function, as we have had some problems where we don't always keep the documentation up to date.

@StevenACoffman StevenACoffman merged commit 182b039 into 99designs:master Sep 2, 2022
@Unkn0wnCat Unkn0wnCat deleted the docs-recipe-subscriptions branch September 3, 2022 15:45
@Unkn0wnCat
Copy link
Contributor Author

Thanks @StevenACoffman! I'll look into if I can write some kind of automated unit-test for the tutorial code, although I don't quite know how I'd go about testing everything at once

@zdraganov
Copy link
Contributor

I have a question. In blog posts from previous versions, I was looking at people awaiting for ctx.Done() to be invoked, but now I see the examples in the docs are awaiting the default of the select.

Is there something that requires the change?

@Unkn0wnCat
Copy link
Contributor Author

@zdraganov
ctx.Done() will probably work too, but I've not tested it. The nice thing about the select default case is if the channel gets closed for whatever reason and a race condition happens, it exits gracefully. If you write to the channel outside of a select case this would cause the Goroutine to panic.

@zdraganov
Copy link
Contributor

zdraganov commented Oct 7, 2022

Thanks for the quick reply!

I will give you my example that we have to use the subscriptions and using ctx.Done() makes it at least more readable.

func (r *subscriptionResolver) ChatMessage(ctx context.Context) (<-chan model.ChatMessage, error) {
        user := utils.GetUserFromContext(ctx)
	channel := r.ChatMessagesBroadcast.Subscribe(user.ID) // this returns the <-chan model.ChatMessage

	go func() {
	     <-ctx.Done()
	     r.ChatMessagesBroadcast.Unsubscribe(user.ID)
	}()

	return channel, nil
}

I have found couple of examples online following this approach, but in that case we have the control at which point to call the Unsubscribe function to properly release the subscription.

If I follow the example from the docs, my implementation should be something similar to:

func (r *subscriptionResolver) ChatMessage(ctx context.Context) (<-chan model.ChatMessage, error) {
        user := utils.GetUserFromContext(ctx)
	channel := r.ChatMessagesBroadcast.Subscribe(user.ID)
	resultChannel := make(<-chan model.ChatMessage)

	go func() {
	    for {
                 msg := <- channel
	         
	         select {
	         case resultChannel <- msg:
	         default:
	              r.ChatMessagesBroadcast.Unsubscribe(user.ID)
	              return
	         }
	    }
	}()

	return resultChannel, nil
}

The example above is not only looking more complex, but can lead to a blocking on msg := <- channel even if the resultsChannel is closed. If we not have a proper ctx.Done() it will be harder to implement proper exit mechanism from the subscriptions.

For me it's even confusing that the library is going to close the returned channel and not giving you the option to release it the way you want.

What do you think? Can you see in the code where actually the returned channel from the subscription is getting deleted?

@Unkn0wnCat
Copy link
Contributor Author

Unkn0wnCat commented Oct 7, 2022

@zdraganov

If the implementation which provides the channel / sends to it already handles safe sending your first solution is good, and yes it's cleaner.

However if the send is unsafe (aka. just sending without any select-esque checks) a race condition could happen due to other Goroutines running in parallel:

go func() {
  <-ctx.Done()
  // Imagine right here the send is happening -
  // The unsubscribe has not yet happened, even though
  //  the context has been finished
  // The send method will now panic and, if not `recover()`-ed
  //  it will take your entire program down.
  r.ChatMessagesBroadcast.Unsubscribe(user.ID)
}()

The example I wrote for the documentation is built with the select-statement to make it as safe as possible out-of-the-box, even for newcomers to channels in Go. If the channel is completely handled somewhere else with a safe send method the select statement if not needed.

@zdraganov
Copy link
Contributor

After couple of load tests, I can confirm that you should check before sending to the channel. There is really a race condition causing send to closed channels even with the <-ctx.Done() approach.

@zdraganov
Copy link
Contributor

zdraganov commented Oct 11, 2022

Actually I found one more issue with the approach in the docs.
If you work with unbuffered channels in the subscriptions and there is in-flight subscription message, when you attempt to send another, you will return from the loop and close the subscription.

@Unkn0wnCat
Copy link
Contributor Author

That's actually a good call, you're completely right on that!
I've overlooked that when writing the example. Maybe you could do a PR adding the buffer? Otherwise I can do it later-ish.

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.

4 participants