From 7ae180bb1eaea8bdfd6d5714aa90b8445165ff1c Mon Sep 17 00:00:00 2001 From: Yuxuan 'fishy' Wang Date: Fri, 5 Aug 2022 15:29:42 -0700 Subject: [PATCH] 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. --- CHANGES.md | 1 + .../tests/json_protocol_deserializer_test.go | 64 +++++++++++++++++++ lib/go/thrift/deserializer.go | 10 +++ lib/go/thrift/serializer.go | 6 ++ lib/go/thrift/simple_json_protocol.go | 14 +++- 5 files changed, 93 insertions(+), 2 deletions(-) create mode 100644 lib/go/test/tests/json_protocol_deserializer_test.go diff --git a/CHANGES.md b/CHANGES.md index 1b451a247bb..f480e42f28b 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 00000000000..468e2c67084 --- /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 cefc7ecda5d..2f2468b2948 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 c44979094c6..f4d920186ad 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 c9c450b89a8..5cefb600abc 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)