Merge pull request #249 from yarpc/dev

Release v0.14.2
yarpc · Jan 31, 2019 · 21e5831 · 21e5831
2 parents cb01c13 + 0f1beae
commit 21e5831
Show file tree

Hide file tree

Showing 9 changed files with 32 additions and 7 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,8 +1,12 @@
 Changelog
 =========
 
+# 0.14.2 (2019-01-31)
+* Fix grpc reflection stream being closed abnormally causing
+  the reflection server to see an unexpected client disconnect.
+
 # 0.14.1 (2018-10-22)
-* Fix a bug for incorrectly dialing the reflection server when 
+* Fix a bug for incorrectly dialing the reflection server when
   peers contain the `grpc://` scheme instead of just host:port pairs.
 
 # 0.14.0 (2018-10-18)

diff --git a/integration_test.go b/integration_test.go
@@ -381,7 +381,7 @@ func setupGRPCServer(t *testing.T) (net.Addr, *grpc.Server) {
 
 func TestGRPCReflectionSource(t *testing.T) {
 	addr, server := setupGRPCServer(t)
-	defer server.Stop()
+	defer server.GracefulStop()
 
 	tests := []struct {
 		desc    string

diff --git a/protobuf/filedescriptorsets.go b/protobuf/filedescriptorsets.go
@@ -79,3 +79,5 @@ func (fs *fileSource) FindSymbol(fullyQualifiedName string) (desc.Descriptor, er
 	}
 	return nil, fmt.Errorf("symbol not found: %q", fullyQualifiedName)
 }
+
+func (fs *fileSource) Close() {}
diff --git a/protobuf/filedescriptorsets_test.go b/protobuf/filedescriptorsets_test.go
@@ -81,8 +81,12 @@ func TestNewDescriptorProviderFileDescriptorSetBins(t *testing.T) {
 				assert.Contains(t, err.Error(), tt.errMsg, "%v: invalid error", tt.name)
 				return
 			}
-			assert.NotNil(t, got)
-			assert.NoError(t, err)
+			require.NoError(t, err)
+			require.NotNil(t, got)
+
+			// Doesn't do anything, but is part of the API.
+			defer got.Close()
+
 			if tt.lookupSymbol != "" {
 				require.NotNil(t, got)
 				s, err := got.FindSymbol(tt.lookupSymbol)

diff --git a/protobuf/source.go b/protobuf/source.go
@@ -8,4 +8,6 @@ import "github.com/jhump/protoreflect/desc"
 type DescriptorProvider interface {
 	// FindSymbol returns a descriptor for the given fully-qualified symbol name.
 	FindSymbol(fullyQualifiedName string) (desc.Descriptor, error)
+
+	Close()
 }
diff --git a/protobuf/source_reflection.go b/protobuf/source_reflection.go
@@ -68,3 +68,7 @@ func (s *grpcreflectSource) FindSymbol(fullyQualifiedName string) (desc.Descript
 	}
 	return file.FindSymbol(fullyQualifiedName), nil
 }
+
+func (s *grpcreflectSource) Close() {
+	s.client.Reset()
+}
diff --git a/protobuf/source_reflection_test.go b/protobuf/source_reflection_test.go
@@ -19,15 +19,20 @@ func TestReflection(t *testing.T) {
 	s := grpc.NewServer()
 	reflection.Register(s)
 	go s.Serve(ln)
-	defer s.Stop()
+
+	// Ensure that all streams are closed by the end of the test.
+	defer s.GracefulStop()
 
 	source, err := NewDescriptorProviderReflection(ReflectionArgs{
 		Timeout: time.Second,
 		Peers:   []string{ln.Addr().String()},
 	})
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	require.NotNil(t, source)
 
+	// Close the streaming reflect call to ensure GracefulStop doesn't block.
+	defer source.Close()
+
 	result, err := source.FindSymbol("grpc.reflection.v1alpha.ServerReflectionRequest")
 	assert.NoError(t, err)
 	assert.NotNil(t, result)

diff --git a/request.go b/request.go
@@ -107,6 +107,10 @@ func NewSerializer(opts Options) (encoding.Serializer, error) {
 		if err != nil {
 			return nil, err
 		}
+
+		// The descriptor is only used in the New function, so it's safe to defer Close.
+		defer descSource.Close()
+
 		return encoding.NewProtobuf(opts.ROpts.Procedure, descSource)
 	}
 

diff --git a/version.go b/version.go
@@ -22,4 +22,4 @@ package main
 
 // versionString is the sem-ver version string for yab.
 // It will be bumped explicitly on releases.
-var versionString = "0.14.1"
+var versionString = "0.14.2"
-Original file line number
+Diff line change
@@ Expand Up @@
     	}
     	return nil, fmt.Errorf("symbol not found: %q", fullyQualifiedName)
     }
+    func (fs *fileSource) Close() {}