Skip to content

Commit 7ae180b

Browse files
Yuxuan 'fishy' Wangfishy
authored andcommitted
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.
1 parent 3f9b7d0 commit 7ae180b

5 files changed

Lines changed: 93 additions & 2 deletions

File tree

CHANGES.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
- [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
99
- [THRIFT-5527](https://issues.apache.org/jira/browse/THRIFT-5527) - Compiler generated Process function will swallow exceptions defined in thrift IDL
1010
- [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
11+
- [THRIFT-5609](https://issues.apache.org/jira/browse/THRIFT-5609) - `TJSONProtocol` is now safe to be used in `TDeserializePool`
1112

1213
## 0.16.0
1314

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* 'License'); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* 'AS IS' BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
package tests
21+
22+
import (
23+
"context"
24+
"testing"
25+
"testing/quick"
26+
27+
"github.com/apache/thrift/lib/go/test/gopath/src/thrifttest"
28+
"github.com/apache/thrift/lib/go/thrift"
29+
)
30+
31+
func TestDeserializerPoolJSONProtocol(t *testing.T) {
32+
ctx := context.Background()
33+
34+
serializerPool := thrift.NewTSerializerPoolSizeFactory(1024, thrift.NewTJSONProtocolFactory())
35+
msg := &thrifttest.Bonk{
36+
Message: "foo",
37+
Type: 42,
38+
}
39+
valid, err := serializerPool.WriteString(ctx, msg)
40+
if err != nil {
41+
t.Fatal(err)
42+
}
43+
invalid := valid[:len(valid)-2]
44+
45+
deserializerPool := thrift.NewTDeserializerPoolSizeFactory(1024, thrift.NewTJSONProtocolFactory())
46+
msg = new(thrifttest.Bonk)
47+
if err := deserializerPool.ReadString(ctx, msg, invalid); err == nil {
48+
t.Fatalf("Deserializing %q did not fail", invalid)
49+
}
50+
51+
f := func() bool {
52+
msg := new(thrifttest.Bonk)
53+
if err := deserializerPool.ReadString(ctx, msg, valid); err != nil {
54+
t.Errorf("Deserializing string %q failed with %v", valid, err)
55+
}
56+
if err := deserializerPool.Read(ctx, msg, []byte(valid)); err != nil {
57+
t.Errorf("Deserializing bytes %q failed with %v", valid, err)
58+
}
59+
return !t.Failed()
60+
}
61+
if err := quick.Check(f, nil); err != nil {
62+
t.Error(err)
63+
}
64+
}

lib/go/thrift/deserializer.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,15 @@ func NewTDeserializer() *TDeserializer {
3939
}
4040
}
4141

42+
type reseter interface {
43+
Reset()
44+
}
45+
4246
func (t *TDeserializer) ReadString(ctx context.Context, msg TStruct, s string) (err error) {
4347
t.Transport.Reset()
48+
if r, ok := t.Protocol.(reseter); ok {
49+
r.Reset()
50+
}
4451

4552
err = nil
4653
if _, err = t.Transport.Write([]byte(s)); err != nil {
@@ -54,6 +61,9 @@ func (t *TDeserializer) ReadString(ctx context.Context, msg TStruct, s string) (
5461

5562
func (t *TDeserializer) Read(ctx context.Context, msg TStruct, b []byte) (err error) {
5663
t.Transport.Reset()
64+
if r, ok := t.Protocol.(reseter); ok {
65+
r.Reset()
66+
}
5767

5868
err = nil
5969
if _, err = t.Transport.Write(b); err != nil {

lib/go/thrift/serializer.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,9 @@ func NewTSerializer() *TSerializer {
4646

4747
func (t *TSerializer) WriteString(ctx context.Context, msg TStruct) (s string, err error) {
4848
t.Transport.Reset()
49+
if r, ok := t.Protocol.(reseter); ok {
50+
r.Reset()
51+
}
4952

5053
if err = msg.Write(ctx, t.Protocol); err != nil {
5154
return
@@ -63,6 +66,9 @@ func (t *TSerializer) WriteString(ctx context.Context, msg TStruct) (s string, e
6366

6467
func (t *TSerializer) Write(ctx context.Context, msg TStruct) (b []byte, err error) {
6568
t.Transport.Reset()
69+
if r, ok := t.Protocol.(reseter); ok {
70+
r.Reset()
71+
}
6672

6773
if err = msg.Write(ctx, t.Protocol); err != nil {
6874
return

lib/go/thrift/simple_json_protocol.go

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -121,8 +121,7 @@ func NewTSimpleJSONProtocolConf(t TTransport, conf *TConfiguration) *TSimpleJSON
121121
writer: bufio.NewWriter(t),
122122
reader: bufio.NewReader(t),
123123
}
124-
v.parseContextStack.push(_CONTEXT_IN_TOPLEVEL)
125-
v.dumpContext.push(_CONTEXT_IN_TOPLEVEL)
124+
v.resetContextStack()
126125
return v
127126
}
128127

@@ -1328,6 +1327,17 @@ func (p *TSimpleJSONProtocol) SetTConfiguration(conf *TConfiguration) {
13281327
p.cfg = conf
13291328
}
13301329

1330+
// Reset resets this protocol's internal state.
1331+
//
1332+
// It's useful when a single protocol instance is reused after errors, to make
1333+
// sure the next use will not be in a bad state to begin with. An example is
1334+
// when it's used in serializer/deserializer pools.
1335+
func (p *TSimpleJSONProtocol) Reset() {
1336+
p.resetContextStack()
1337+
p.writer.Reset(p.trans)
1338+
p.reader.Reset(p.trans)
1339+
}
1340+
13311341
var (
13321342
_ TConfigurationSetter = (*TSimpleJSONProtocol)(nil)
13331343
_ TConfigurationSetter = (*TSimpleJSONProtocolFactory)(nil)

0 commit comments

Comments
 (0)