Repository: Khan/genqlient
Pull request #384 (2025-06-11 15:38:40)

[Subscription] check already closed channels 
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



Patch:
diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md
index 1cee26a..f5bbc85 100644
--- a/docs/CHANGELOG.md
+++ b/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
 
diff --git a/graphql/subscription.go b/graphql/subscription.go
index f86f75e..8e20ba5 100644
--- a/graphql/subscription.go
+++ b/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
 }
 
diff --git a/internal/lint/go.mod b/internal/lint/go.mod
index 7dc2070..c5f6999 100644
--- a/internal/lint/go.mod
+++ b/internal/lint/go.mod
@@ -60,7 +60,7 @@ require (
 	github.com/go-toolsmith/astp v1.1.0 // indirect
 	github.com/go-toolsmith/strparse v1.1.0 // indirect
 	github.com/go-toolsmith/typep v1.1.0 // indirect
-	github.com/go-viper/mapstructure/v2 v2.2.1 // indirect
+	github.com/go-viper/mapstructure/v2 v2.3.0 // indirect
 	github.com/go-xmlfmt/xmlfmt v1.1.2 // indirect
 	github.com/gobwas/glob v0.2.3 // indirect
 	github.com/gofrs/flock v0.12.1 // indirect
diff --git a/internal/lint/go.sum b/internal/lint/go.sum
index 20fd868..3aefa53 100644
--- a/internal/lint/go.sum
+++ b/internal/lint/go.sum
@@ -186,8 +186,8 @@ github.com/go-toolsmith/strparse v1.1.0 h1:GAioeZUK9TGxnLS+qfdqNbA4z0SSm5zVNtCQi
 github.com/go-toolsmith/strparse v1.1.0/go.mod h1:7ksGy58fsaQkGQlY8WVoBFNyEPMGuJin1rfoPS4lBSQ=
 github.com/go-toolsmith/typep v1.1.0 h1:fIRYDyF+JywLfqzyhdiHzRop/GQDxxNhLGQ6gFUNHus=
 github.com/go-toolsmith/typep v1.1.0/go.mod h1:fVIw+7zjdsMxDA3ITWnH1yOiw1rnTQKCsF/sk2H/qig=
-github.com/go-viper/mapstructure/v2 v2.2.1 h1:ZAaOCxANMuZx5RCeg0mBdEZk7DZasvvZIxtHqx8aGss=
-github.com/go-viper/mapstructure/v2 v2.2.1/go.mod h1:oJDH3BJKyqBA2TXFhDsKDGDTlndYOZ6rGS0BRZIxGhM=
+github.com/go-viper/mapstructure/v2 v2.3.0 h1:27XbWsHIqhbdR5TIC911OfYvgSaW93HM+dX7970Q7jk=
+github.com/go-viper/mapstructure/v2 v2.3.0/go.mod h1:oJDH3BJKyqBA2TXFhDsKDGDTlndYOZ6rGS0BRZIxGhM=
 github.com/go-xmlfmt/xmlfmt v1.1.2 h1:Nea7b4icn8s57fTx1M5AI4qQT5HEM3rVUO8MuE6g80U=
 github.com/go-xmlfmt/xmlfmt v1.1.2/go.mod h1:aUCEOzzezBEjDBbFBoSiya/gduyIiWYRP6CnSFIV8AM=
 github.com/gobwas/glob v0.2.3 h1:A4xDbljILXROh+kObIiy5kIaPYD8e96x1tgBhUI5J+Y=


Test patch:
diff --git a/graphql/subscription_test.go b/graphql/subscription_test.go
new file mode 100644
index 0000000..f027001
--- /dev/null
+++ b/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)
+			}
+		})
+	}
+}