Subscription support

[benjaminjkraft]

Added support for graphQL subscriptions.

• Using graphql.NewClientUsingWebSocket(), the returned graphql.WebSocketClient will be able to subscribe to graphQL endpoints.
• Implementation does not depend on a specific websocket package, but it is similar to github.com/gorilla/websocket (this package is also used to create a client in the tests).

Note: this is just #294 with main merged and a few small docs tweaks. See there for review discussion etc.

Fixes #199.

I have:

• Written a clear PR title and description (above)
• Signed the Khan Academy CLA
• Added tests covering my changes, if applicable
• Included a link to the issue fixed, if applicable
• Included documentation, for new features
• Added an entry to the changelog

[HaraldNordgren]

Awesome work, @benjaminjkraft! We have been waiting for this one! 🚀

Do you know who is in charge of generating new releases, hopefully we can get a v0.8.0 or similar with this included, so we can start using it 🤗

[HaraldNordgren]

@benjaminjkraft Any updates on when v0.8.0 will be released?

[HaraldNordgren]

@benjaminjkraft, I wonder if there is an issue in this PR. It the test cases, the data channel in never closed. When I close it in my resolver, I get JSON unmarshaling errors.

It seems maybe the we try to unmarshal the output of a closed channel. I'm trying to reproduce the issue here, maybe you can approve the workflows to run to test it out: #353

I have to add a special case to my tests to make them pass.

			actual := []*genqlient.StreamChatWithBotResponse{}

			for loop := true; loop; {
				select {
				case resp, more := <-dataChan:
					if !more {
						loop = false
						break
					}
					require.NoError(t, resp.Errors)
					actual = append(actual, resp.Data)

				case err = <-errChan:
					// Special case: It seems that genqlient returns this error when the channel is closed from the resolver side
					if err.Error() == "unexpected end of JSON input" {
						continue
					}

					require.NoError(t, err)
					loop = false

				case <-time.After(3 * time.Second):
					err := wsClient.Unsubscribe(subscriptionID)
					require.NoError(t, err)
					loop = false
				}
			}

			// Assert

			assert.Equal(t, c.expected, actual)

[matthieu4294967296moineau]

the data channel in never closed. When I close it in my resolver, I get JSON unmarshaling errors.

I am not sure how to reproduce your problem, but the dataChan is closed when calling WebSocketClient.Unsubscribe.
Is it not what you see ?
If not, can you write a full script so I can try and reproduce the same problem you have ?

[benjaminjkraft]

It looks like the tests on #353 do show the issue, but @matthieu4294967296moineau may need to comment as to whether this is something to clarify in the documentation, or a bug!

[HaraldNordgren]

Thanks for the quick replies and for starting the workflow runs! ❤️

The issue I'm a getting is a different one than my PR currently has, I will take another look to try to reproduce what is happing in our own code base, which is not the server panicking when writing on a closed channel.

But instead the client is getting a JSON unmarshaling error, after having successfully read all the data. It looks like the genqlient attempts to unmarshal one last time after all the data has already been consumed and the unmsrshal of empty data fails.

As a reference, we followed this when implementation our resolver. We use gqlgen for resolvers and genqlient for testing, which has served us well so far. Notice how the channel closing is performed:
https://gqlgen.com/recipes/subscriptions/#implementing-your-resolver

[HaraldNordgren]

It's my bad for never attempting to run the tests locally before pushing to remote. This is not the correct error unfortunately: link

@benjaminjkraft @matthieu4294967296moineau I have pushed more code to my PR (#353) it should now give this error which I am receiving locally. Could you please approve the workflows to run?

=== RUN   TestSubscription
    integration_test.go:94:
        	Error Trace:	/Users/Harald/git-repos/github.com/Khan/genqlient/internal/integration/integration_test.go:94
        	Error:      	Received unexpected error:
        	            	unexpected end of JSON input
        	Test:       	TestSubscription

[HaraldNordgren]

Maybe something like this can be a step in the right direction for solving this issue?

#354 (Handle websocket data channel closing)

[matthieu4294967296moineau]

The way the subscription works for now is that, a client subscribes, and gets data on dataChan until the client himself decides to unsubscribe, which will trigger close(dataChan) from.
If you want to end the subscription from inside the subscription, it requires some changes in the way dataChan is handled.
Is this what you are trying to achieve ?

[HaraldNordgren]

If you want to end the subscription from inside the subscription, it requires some changes in the way dataChan is handled.
Is this what you are trying to achieve ?

Yeah, that sounds like it 👍

As I mentioned above, we are using gqlgen for the server and closing the channel like they are suggesting:

we followed this when implementation our resolver. We use gqlgen for resolvers and genqlient for testing, which has served us well so far. Notice how the channel closing is performed:
https://gqlgen.com/recipes/subscriptions/#implementing-your-resolver

I also connected to this gqlgen from Postman, and it seems to work. It sends the data and closes without error messages:

Correct me if I'm wrong, but websocket is a two-way communication. So the client must be prepared that the server closes the data channel, no? 🤗

[matthieu4294967296moineau]

Yes you are right, I was just never using it in this way, this is why it is missing in my previous PR.
I can have a look at how to fix it next week, or maybe your fix is already good 👍.

[benjaminjkraft]

Ugh, this is where I wish there were a clearer spec for GraphQL's transport layer, these things all end up being so implementation-defined and everyone has to figure them all out again. Anyway, I agree if the server closes the websocket we had better handle that!

[HaraldNordgren]

@benjaminjkraft @matthieu4294967296moineau I believe this is good enough for a review now, if you want to take a look: #354