Skip to content

Commit 9605bd5

Browse files
asmyasnikovasmyasnikov
authored
Merge pull request #1394 from ydb-platform/issue#1364
* Fixed bug with nil pointer dereference on trace callback from `query.createSession`
2 parents 846b6ed + af9ae30 commit 9605bd5

File tree

3 files changed

+139
-21
lines changed

3 files changed

+139
-21
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
* Fixed bug with nil pointer dereference on trace callback from `query.createSession`
12
* Fixed test message builder, now all method return itself pointer
23
* Fixed handle reconnection timeout error
34
* Fixed experimental topic listener handle stop partition event

‎internal/query/session.go‎

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -89,35 +89,37 @@ func createSession(ctx context.Context, client Ydb_Query_V1.QueryServiceClient,
8989
},
9090
},
9191
}
92-
defer func() {
93-
if finalErr != nil && s != nil {
94-
panic("abnormal result")
95-
}
96-
}()
9792

9893
onDone := trace.QueryOnSessionCreate(s.cfg.Trace(), &ctx,
9994
stack.FunctionID("github.com/ydb-platform/ydb-go-sdk/3/internal/query.createSession"),
10095
)
10196
defer func() {
102-
onDone(s, finalErr)
97+
if s == nil && finalErr == nil {
98+
panic("abnormal result: both nil")
99+
}
100+
if s != nil && finalErr != nil {
101+
panic("abnormal result: both not nil")
102+
}
103+
104+
if finalErr != nil {
105+
onDone(nil, finalErr)
106+
} else if s != nil {
107+
onDone(s, nil)
108+
}
103109
}()
104110

105111
response, err := client.CreateSession(ctx, &Ydb_Query.CreateSessionRequest{})
106112
if err != nil {
107113
return nil, xerrors.WithStackTrace(err)
108114
}
109115

110-
defer func() {
111-
if finalErr != nil {
112-
_ = deleteSession(ctx, client, response.GetSessionId())
113-
}
114-
}()
115-
116116
s.id = response.GetSessionId()
117117
s.nodeID = response.GetNodeId()
118118

119119
err = s.attach(ctx)
120120
if err != nil {
121+
_ = deleteSession(ctx, client, response.GetSessionId())
122+
121123
return nil, xerrors.WithStackTrace(err)
122124
}
123125

‎internal/query/session_test.go‎

Lines changed: 124 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -10,47 +10,162 @@ import (
1010
grpcCodes "google.golang.org/grpc/codes"
1111
grpcStatus "google.golang.org/grpc/status"
1212

13+
"github.com/ydb-platform/ydb-go-sdk/v3/internal/query/config"
1314
"github.com/ydb-platform/ydb-go-sdk/v3/internal/xerrors"
1415
"github.com/ydb-platform/ydb-go-sdk/v3/internal/xtest"
1516
"github.com/ydb-platform/ydb-go-sdk/v3/query"
17+
"github.com/ydb-platform/ydb-go-sdk/v3/trace"
1618
)
1719

1820
func TestBegin(t *testing.T) {
1921
t.Run("HappyWay", func(t *testing.T) {
2022
ctx := xtest.Context(t)
2123
ctrl := gomock.NewController(t)
22-
service := NewMockQueryServiceClient(ctrl)
23-
service.EXPECT().BeginTransaction(gomock.Any(), gomock.Any()).Return(&Ydb_Query.BeginTransactionResponse{
24+
client := NewMockQueryServiceClient(ctrl)
25+
client.EXPECT().BeginTransaction(gomock.Any(), gomock.Any()).Return(&Ydb_Query.BeginTransactionResponse{
2426
Status: Ydb.StatusIds_SUCCESS,
2527
TxMeta: &Ydb_Query.TransactionMeta{
2628
Id: "123",
2729
},
2830
}, nil)
2931
t.Log("begin")
30-
tx, err := begin(ctx, service, &Session{id: "123"}, query.TxSettings())
32+
tx, err := begin(ctx, client, &Session{id: "123"}, query.TxSettings())
3133
require.NoError(t, err)
3234
require.Equal(t, "123", tx.ID())
3335
})
3436
t.Run("TransportError", func(t *testing.T) {
3537
ctx := xtest.Context(t)
3638
ctrl := gomock.NewController(t)
37-
service := NewMockQueryServiceClient(ctrl)
38-
service.EXPECT().BeginTransaction(gomock.Any(), gomock.Any()).Return(nil, grpcStatus.Error(grpcCodes.Unavailable, ""))
39+
client := NewMockQueryServiceClient(ctrl)
40+
client.EXPECT().BeginTransaction(gomock.Any(), gomock.Any()).Return(nil, grpcStatus.Error(grpcCodes.Unavailable, ""))
3941
t.Log("begin")
40-
_, err := begin(ctx, service, &Session{id: "123"}, query.TxSettings())
42+
_, err := begin(ctx, client, &Session{id: "123"}, query.TxSettings())
4143
require.Error(t, err)
4244
require.True(t, xerrors.IsTransportError(err, grpcCodes.Unavailable))
4345
})
4446
t.Run("OperationError", func(t *testing.T) {
4547
ctx := xtest.Context(t)
4648
ctrl := gomock.NewController(t)
47-
service := NewMockQueryServiceClient(ctrl)
48-
service.EXPECT().BeginTransaction(gomock.Any(), gomock.Any()).Return(nil,
49+
client := NewMockQueryServiceClient(ctrl)
50+
client.EXPECT().BeginTransaction(gomock.Any(), gomock.Any()).Return(nil,
4951
xerrors.Operation(xerrors.WithStatusCode(Ydb.StatusIds_UNAVAILABLE)),
5052
)
5153
t.Log("begin")
52-
_, err := begin(ctx, service, &Session{id: "123"}, query.TxSettings())
54+
_, err := begin(ctx, client, &Session{id: "123"}, query.TxSettings())
5355
require.Error(t, err)
5456
require.True(t, xerrors.IsOperationError(err, Ydb.StatusIds_UNAVAILABLE))
5557
})
5658
}
59+
60+
func TestCreateSession(t *testing.T) {
61+
trace := &trace.Query{
62+
OnSessionCreate: func(info trace.QuerySessionCreateStartInfo) func(info trace.QuerySessionCreateDoneInfo) {
63+
return func(info trace.QuerySessionCreateDoneInfo) {
64+
if info.Session != nil && info.Error != nil {
65+
panic("only one result from tuple may be not nil")
66+
}
67+
}
68+
},
69+
}
70+
t.Run("HappyWay", func(t *testing.T) {
71+
ctx := xtest.Context(t)
72+
ctrl := gomock.NewController(t)
73+
client := NewMockQueryServiceClient(ctrl)
74+
client.EXPECT().CreateSession(gomock.Any(), gomock.Any()).Return(&Ydb_Query.CreateSessionResponse{
75+
Status: Ydb.StatusIds_SUCCESS,
76+
SessionId: "123",
77+
}, nil)
78+
attachStream := NewMockQueryService_AttachSessionClient(ctrl)
79+
attachStream.EXPECT().Recv().Return(&Ydb_Query.SessionState{
80+
Status: Ydb.StatusIds_SUCCESS,
81+
}, nil).AnyTimes()
82+
client.EXPECT().AttachSession(gomock.Any(), &Ydb_Query.AttachSessionRequest{
83+
SessionId: "123",
84+
}).Return(attachStream, nil)
85+
t.Log("createSession")
86+
require.NotPanics(t, func() {
87+
s, err := createSession(ctx, client, config.New(config.WithTrace(trace)))
88+
require.NoError(t, err)
89+
require.NotNil(t, s)
90+
require.Equal(t, "123", s.ID())
91+
})
92+
})
93+
t.Run("TransportError", func(t *testing.T) {
94+
t.Run("OnCreateSession", func(t *testing.T) {
95+
ctx := xtest.Context(t)
96+
ctrl := gomock.NewController(t)
97+
client := NewMockQueryServiceClient(ctrl)
98+
client.EXPECT().CreateSession(gomock.Any(), gomock.Any()).Return(nil,
99+
xerrors.Transport(grpcStatus.Error(grpcCodes.Unavailable, "test")),
100+
)
101+
t.Log("createSession")
102+
require.NotPanics(t, func() {
103+
s, err := createSession(ctx, client, config.New(config.WithTrace(trace)))
104+
require.Error(t, err)
105+
require.Nil(t, s)
106+
})
107+
})
108+
t.Run("OnAttachStream", func(t *testing.T) {
109+
ctx := xtest.Context(t)
110+
ctrl := gomock.NewController(t)
111+
client := NewMockQueryServiceClient(ctrl)
112+
client.EXPECT().CreateSession(gomock.Any(), gomock.Any()).Return(&Ydb_Query.CreateSessionResponse{
113+
Status: Ydb.StatusIds_SUCCESS,
114+
SessionId: "123",
115+
}, nil)
116+
client.EXPECT().AttachSession(gomock.Any(), &Ydb_Query.AttachSessionRequest{
117+
SessionId: "123",
118+
}).Return(nil, xerrors.Transport(grpcStatus.Error(grpcCodes.Unavailable, "test")))
119+
client.EXPECT().DeleteSession(gomock.Any(), &Ydb_Query.DeleteSessionRequest{
120+
SessionId: "123",
121+
}).Return(&Ydb_Query.DeleteSessionResponse{
122+
Status: Ydb.StatusIds_SUCCESS,
123+
}, nil)
124+
t.Log("createSession")
125+
require.NotPanics(t, func() {
126+
s, err := createSession(ctx, client, config.New(config.WithTrace(trace)))
127+
require.Error(t, err)
128+
require.Nil(t, s)
129+
})
130+
})
131+
})
132+
t.Run("OperationError", func(t *testing.T) {
133+
t.Run("OnCreateSession", func(t *testing.T) {
134+
ctx := xtest.Context(t)
135+
ctrl := gomock.NewController(t)
136+
client := NewMockQueryServiceClient(ctrl)
137+
client.EXPECT().CreateSession(gomock.Any(), gomock.Any()).Return(nil,
138+
xerrors.Operation(xerrors.WithStatusCode(Ydb.StatusIds_UNAVAILABLE)),
139+
)
140+
t.Log("createSession")
141+
require.NotPanics(t, func() {
142+
s, err := createSession(ctx, client, config.New(config.WithTrace(trace)))
143+
require.Error(t, err)
144+
require.Nil(t, s)
145+
})
146+
})
147+
t.Run("OnAttachStream", func(t *testing.T) {
148+
ctx := xtest.Context(t)
149+
ctrl := gomock.NewController(t)
150+
client := NewMockQueryServiceClient(ctrl)
151+
client.EXPECT().CreateSession(gomock.Any(), gomock.Any()).Return(&Ydb_Query.CreateSessionResponse{
152+
Status: Ydb.StatusIds_SUCCESS,
153+
SessionId: "123",
154+
}, nil)
155+
client.EXPECT().AttachSession(gomock.Any(), &Ydb_Query.AttachSessionRequest{
156+
SessionId: "123",
157+
}).Return(nil, xerrors.Operation(xerrors.WithStatusCode(Ydb.StatusIds_UNAVAILABLE)))
158+
client.EXPECT().DeleteSession(gomock.Any(), &Ydb_Query.DeleteSessionRequest{
159+
SessionId: "123",
160+
}).Return(&Ydb_Query.DeleteSessionResponse{
161+
Status: Ydb.StatusIds_SUCCESS,
162+
}, nil)
163+
t.Log("createSession")
164+
require.NotPanics(t, func() {
165+
s, err := createSession(ctx, client, config.New(config.WithTrace(trace)))
166+
require.Error(t, err)
167+
require.Nil(t, s)
168+
})
169+
})
170+
})
171+
}

0 commit comments

Comments
 (0)