[Subscription] check already closed channels (#384)

This PR fixes an issue I encountered using subscriptions with genqlient.

I've noticed that the Unsubscribe function closes the subscription
channel, but never checks if that channel has already been closed.

So a snippet like that causes a panic when Close is called:

``` go
err = gqlsubcl.Unsubscribe(subscriptionID)
if err != nil {
	return fmt.Errorf("failed to unsubscribe from GraphQL subscription: %w", err)
}

if err := gqlsubcl.Close(); err != nil {
	return fmt.Errorf("failed to close GraphQL subscription client: %w", err)
}
```

I have:
- [x] Written a clear PR title and description (above)
- [x] Signed the [Khan Academy CLA](https://www.khanacademy.org/r/cla)
- [x] Added tests covering my changes, if applicable
- [ ] Included a link to the issue fixed, if applicable
- [x] Included documentation, for new features
- [x] Added an entry to the changelog

Author: cheina97

docs/CHANGELOG.md

@@ -41,6 +41,7 @@ This release fixes a bug introduced in v0.8.0 breaking path resolution on Window
 - fixed documentation link in `introduction.md`
 - upgraded version of alexflint/go-arg from 1.4.2 to 1.5.1
 - fixed a typo in the struct + fragment error message
+- avoid closing subscription channels more than once, which could cause a panic in some cases
 
 ## v0.8.0
 

graphql/subscription.go

@@ -44,9 +44,13 @@ func (s *subscriptionMap) Unsubscribe(subscriptionID string) error {
 	if !success {
 		return fmt.Errorf("tried to unsubscribe from unknown subscription with ID '%s'", subscriptionID)
 	}
+	hasBeenUnsubscribed := unsub.hasBeenUnsubscribed
 	unsub.hasBeenUnsubscribed = true
 	s.map_[subscriptionID] = unsub
-	reflect.ValueOf(s.map_[subscriptionID].interfaceChan).Close()
+
+	if !hasBeenUnsubscribed {
+		reflect.ValueOf(s.map_[subscriptionID].interfaceChan).Close()
+	}
 	return nil
 }
 

graphql/subscription_test.go

@@ -0,0 +1,65 @@
+package graphql
+
+import (
+	"testing"
+)
+
+func Test_subscriptionMap_Unsubscribe(t *testing.T) {
+	type args struct {
+		subscriptionID string
+	}
+	tests := []struct {
+		name    string
+		args    args
+		sm      subscriptionMap
+		wantErr bool
+	}{
+		{
+			name: "unsubscribe existing subscription",
+			sm: subscriptionMap{
+				map_: map[string]subscription{
+					"sub1": {
+						id:                  "sub1",
+						interfaceChan:       make(chan struct{}),
+						forwardDataFunc:     nil,
+						hasBeenUnsubscribed: false,
+					},
+				},
+			},
+			args:    args{subscriptionID: "sub1"},
+			wantErr: false,
+		},
+		{
+			name: "unsubscribe non-existent subscription",
+			sm: subscriptionMap{
+				map_: map[string]subscription{},
+			},
+			args:    args{subscriptionID: "doesnotexist"},
+			wantErr: true,
+		},
+		{
+			name: "unsubscribe already unsubscribed subscription",
+			sm: subscriptionMap{
+				map_: map[string]subscription{
+					"sub2": {
+						id:                  "sub2",
+						interfaceChan:       nil,
+						forwardDataFunc:     nil,
+						hasBeenUnsubscribed: true,
+					},
+				},
+			},
+			args:    args{subscriptionID: "sub2"},
+			wantErr: false,
+		},
+	}
+	for i := range tests {
+		tt := &tests[i]
+		t.Run(tt.name, func(t *testing.T) {
+			s := &tt.sm
+			if err := s.Unsubscribe(tt.args.subscriptionID); (err != nil) != tt.wantErr {
+				t.Errorf("subscriptionMap.Unsubscribe() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}