THRIFT-5609: Make TJSONProtocol safe to be used in deserializer pool

Client: go

Add Reset to TJSONProtocol, and call it in deserializer and serializer
to make sure that it's always safe to be used in the pool version.

5 files changed, 93 insertions, 2 deletions

diff --git a/CHANGES.md b/CHANGES.md
index 1b451a247..f480e42f2 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -8,6 +8,7 @@
 - [THRIFT-5583](https://issues.apache.org/jira/browse/THRIFT-5583) - Add `skip_remote` arg to compiler, which can be used to skip the generating of -remote folders for services
 - [THRIFT-5527](https://issues.apache.org/jira/browse/THRIFT-5527) - Compiler generated Process function will swallow exceptions defined in thrift IDL
 - [THRIFT-5605](https://issues.apache.org/jira/browse/THRIFT-5605) - Provide `ExtractIDLExceptionClientMiddleware` and `ExtractExceptionFromResult` to help client middlewares to gain access to exceptions defined in thrift IDL
+- [THRIFT-5609](https://issues.apache.org/jira/browse/THRIFT-5609) - `TJSONProtocol` is now safe to be used in `TDeserializePool`
 
 ## 0.16.0
 
diff --git a/lib/go/test/tests/json_protocol_deserializer_test.go b/lib/go/test/tests/json_protocol_deserializer_test.go
new file mode 100644
index 000000000..468e2c670
--- /dev/null
+++ b/lib/go/test/tests/json_protocol_deserializer_test.go
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * 'License'); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package tests
+
+import (
+	"context"
+	"testing"
+	"testing/quick"
+
+	"github.com/apache/thrift/lib/go/test/gopath/src/thrifttest"
+	"github.com/apache/thrift/lib/go/thrift"
+)
+
+func TestDeserializerPoolJSONProtocol(t *testing.T) {
+	ctx := context.Background()
+
+	serializerPool := thrift.NewTSerializerPoolSizeFactory(1024, thrift.NewTJSONProtocolFactory())
+	msg := &thrifttest.Bonk{
+		Message: "foo",
+		Type:    42,
+	}
+	valid, err := serializerPool.WriteString(ctx, msg)
+	if err != nil {
+		t.Fatal(err)
+	}
+	invalid := valid[:len(valid)-2]
+
+	deserializerPool := thrift.NewTDeserializerPoolSizeFactory(1024, thrift.NewTJSONProtocolFactory())
+	msg = new(thrifttest.Bonk)
+	if err := deserializerPool.ReadString(ctx, msg, invalid); err == nil {
+		t.Fatalf("Deserializing %q did not fail", invalid)
+	}
+
+	f := func() bool {
+		msg := new(thrifttest.Bonk)
+		if err := deserializerPool.ReadString(ctx, msg, valid); err != nil {
+			t.Errorf("Deserializing string %q failed with %v", valid, err)
+		}
+		if err := deserializerPool.Read(ctx, msg, []byte(valid)); err != nil {
+			t.Errorf("Deserializing bytes %q failed with %v", valid, err)
+		}
+		return !t.Failed()
+	}
+	if err := quick.Check(f, nil); err != nil {
+		t.Error(err)
+	}
+}
diff --git a/lib/go/thrift/deserializer.go b/lib/go/thrift/deserializer.go
index cefc7ecda..2f2468b29 100644
--- a/lib/go/thrift/deserializer.go
+++ b/lib/go/thrift/deserializer.go
@@ -39,8 +39,15 @@ func NewTDeserializer() *TDeserializer {
 	}
 }
 
+type reseter interface {
+	Reset()
+}
+
 func (t *TDeserializer) ReadString(ctx context.Context, msg TStruct, s string) (err error) {
 	t.Transport.Reset()
+	if r, ok := t.Protocol.(reseter); ok {
+		r.Reset()
+	}
 
 	err = nil
 	if _, err = t.Transport.Write([]byte(s)); err != nil {
@@ -54,6 +61,9 @@ func (t *TDeserializer) ReadString(ctx context.Context, msg TStruct, s string) (
 
 func (t *TDeserializer) Read(ctx context.Context, msg TStruct, b []byte) (err error) {
 	t.Transport.Reset()
+	if r, ok := t.Protocol.(reseter); ok {
+		r.Reset()
+	}
 
 	err = nil
 	if _, err = t.Transport.Write(b); err != nil {
diff --git a/lib/go/thrift/serializer.go b/lib/go/thrift/serializer.go
index c44979094..f4d920186 100644
--- a/lib/go/thrift/serializer.go
+++ b/lib/go/thrift/serializer.go
@@ -46,6 +46,9 @@ func NewTSerializer() *TSerializer {
 
 func (t *TSerializer) WriteString(ctx context.Context, msg TStruct) (s string, err error) {
 	t.Transport.Reset()
+	if r, ok := t.Protocol.(reseter); ok {
+		r.Reset()
+	}
 
 	if err = msg.Write(ctx, t.Protocol); err != nil {
 		return
@@ -63,6 +66,9 @@ func (t *TSerializer) WriteString(ctx context.Context, msg TStruct) (s string, e
 
 func (t *TSerializer) Write(ctx context.Context, msg TStruct) (b []byte, err error) {
 	t.Transport.Reset()
+	if r, ok := t.Protocol.(reseter); ok {
+		r.Reset()
+	}
 
 	if err = msg.Write(ctx, t.Protocol); err != nil {
 		return
diff --git a/lib/go/thrift/simple_json_protocol.go b/lib/go/thrift/simple_json_protocol.go
index c9c450b89..5cefb600a 100644
--- a/lib/go/thrift/simple_json_protocol.go
+++ b/lib/go/thrift/simple_json_protocol.go
@@ -121,8 +121,7 @@ func NewTSimpleJSONProtocolConf(t TTransport, conf *TConfiguration) *TSimpleJSON
 		writer: bufio.NewWriter(t),
 		reader: bufio.NewReader(t),
 	}
-	v.parseContextStack.push(_CONTEXT_IN_TOPLEVEL)
-	v.dumpContext.push(_CONTEXT_IN_TOPLEVEL)
+	v.resetContextStack()
 	return v
 }
 
@@ -1328,6 +1327,17 @@ func (p *TSimpleJSONProtocol) SetTConfiguration(conf *TConfiguration) {
 	p.cfg = conf
 }
 
+// Reset resets this protocol's internal state.
+//
+// It's useful when a single protocol instance is reused after errors, to make
+// sure the next use will not be in a bad state to begin with. An example is
+// when it's used in serializer/deserializer pools.
+func (p *TSimpleJSONProtocol) Reset() {
+	p.resetContextStack()
+	p.writer.Reset(p.trans)
+	p.reader.Reset(p.trans)
+}
+
 var (
 	_ TConfigurationSetter = (*TSimpleJSONProtocol)(nil)
 	_ TConfigurationSetter = (*TSimpleJSONProtocolFactory)(nil)