From 086f019b43b46778f23cbc2ea071023d846a6c70 Mon Sep 17 00:00:00 2001 From: denis-tingajkin Date: Tue, 31 Mar 2020 13:25:47 +0700 Subject: [PATCH 1/5] fix issue #172 Signed-off-by: denis-tingajkin --- pkg/networkservice/core/next/client.go | 23 ++++++++++++------- pkg/networkservice/core/next/server.go | 18 +++++++++++---- .../core/next/tests/client_test.go | 4 +++- .../core/next/tests/server_test.go | 4 +++- 4 files changed, 34 insertions(+), 15 deletions(-) diff --git a/pkg/networkservice/core/next/client.go b/pkg/networkservice/core/next/client.go index 0f5f5dc05..7479e6918 100644 --- a/pkg/networkservice/core/next/client.go +++ b/pkg/networkservice/core/next/client.go @@ -20,7 +20,6 @@ package next import ( "context" - "github.com/golang/protobuf/ptypes/empty" "google.golang.org/grpc" @@ -40,11 +39,9 @@ type ClientChainer func(...networkservice.NetworkServiceClient) networkservice.N // NewWrappedNetworkServiceClient chains together clients with wrapper wrapped around each one func NewWrappedNetworkServiceClient(wrapper ClientWrapper, clients ...networkservice.NetworkServiceClient) networkservice.NetworkServiceClient { - rv := &nextClient{ - clients: clients, - } - for i := range rv.clients { - rv.clients[i] = wrapper(rv.clients[i]) + rv := &nextClient{} + for _, c := range clients { + appendClient(rv, wrapper, c) } return rv } @@ -54,7 +51,7 @@ func NewNetworkServiceClient(clients ...networkservice.NetworkServiceClient) net if len(clients) == 0 { return &tailClient{} } - return NewWrappedNetworkServiceClient(notWrapClient, clients...) + return NewWrappedNetworkServiceClient(defaultWrapper, clients...) } func (n *nextClient) Request(ctx context.Context, request *networkservice.NetworkServiceRequest, opts ...grpc.CallOption) (*networkservice.Connection, error) { @@ -71,6 +68,16 @@ func (n *nextClient) Close(ctx context.Context, conn *networkservice.Connection, return n.clients[n.index].Close(withNextClient(ctx, nil), conn, opts...) } -func notWrapClient(c networkservice.NetworkServiceClient) networkservice.NetworkServiceClient { +func defaultWrapper(c networkservice.NetworkServiceClient) networkservice.NetworkServiceClient { return c } + +func appendClient(next *nextClient, wrapper ClientWrapper, client networkservice.NetworkServiceClient) { + if v, ok := client.(*nextClient); ok { + for _, c := range v.clients { + appendClient(next, wrapper, c) + } + return + } + next.clients = append(next.clients, wrapper(client)) +} diff --git a/pkg/networkservice/core/next/server.go b/pkg/networkservice/core/next/server.go index 8f2bb75db..3bc895d06 100644 --- a/pkg/networkservice/core/next/server.go +++ b/pkg/networkservice/core/next/server.go @@ -39,11 +39,9 @@ type ServerChainer func(...networkservice.NetworkServiceServer) networkservice.N // NewWrappedNetworkServiceServer - chains together the servers provides with the wrapper wrapped around each one in turn. func NewWrappedNetworkServiceServer(wrapper ServerWrapper, servers ...networkservice.NetworkServiceServer) networkservice.NetworkServiceServer { - rv := &nextServer{ - servers: servers, - } - for i := range rv.servers { - rv.servers[i] = wrapper(rv.servers[i]) + rv := &nextServer{} + for _, s := range servers { + appendServer(rv, wrapper, s) } return rv } @@ -74,3 +72,13 @@ func (n *nextServer) Close(ctx context.Context, conn *networkservice.Connection) func notWrapServer(server networkservice.NetworkServiceServer) networkservice.NetworkServiceServer { return server } + +func appendServer(next *nextServer, wrapper ServerWrapper, server networkservice.NetworkServiceServer) { + if v, ok := server.(*nextServer); ok { + for _, c := range v.servers { + appendServer(next, wrapper, c) + } + return + } + next.servers = append(next.servers, wrapper(server)) +} diff --git a/pkg/networkservice/core/next/tests/client_test.go b/pkg/networkservice/core/next/tests/client_test.go index 8c36b0722..36a10fd78 100644 --- a/pkg/networkservice/core/next/tests/client_test.go +++ b/pkg/networkservice/core/next/tests/client_test.go @@ -48,8 +48,10 @@ func TestClientBranches(t *testing.T) { {visitClient(), visitClient(), adapters.NewServerToClient(visitServer())}, {visitClient(), adapters.NewServerToClient(emptyServer()), visitClient()}, {visitClient(), adapters.NewServerToClient(visitServer()), emptyClient()}, + {visitClient(), next.NewNetworkServiceClient(next.NewNetworkServiceClient(visitClient())), visitClient()}, + {visitClient(), next.NewNetworkServiceClient(next.NewNetworkServiceClient(emptyClient())), visitClient()}, } - expects := []int{1, 2, 3, 0, 1, 2, 1, 2, 3, 3, 1, 2} + expects := []int{1, 2, 3, 0, 1, 2, 1, 2, 3, 3, 1, 2, 3, 1} for i, sample := range samples { msg := fmt.Sprintf("sample index: %v", i) ctx := visit(context.Background()) diff --git a/pkg/networkservice/core/next/tests/server_test.go b/pkg/networkservice/core/next/tests/server_test.go index ceca795b4..c6fd113de 100644 --- a/pkg/networkservice/core/next/tests/server_test.go +++ b/pkg/networkservice/core/next/tests/server_test.go @@ -50,8 +50,10 @@ func TestServerBranches(t *testing.T) { {visitServer(), visitServer(), adapters.NewClientToServer(visitClient())}, {visitServer(), adapters.NewClientToServer(emptyClient()), visitServer()}, {visitServer(), adapters.NewClientToServer(visitClient()), emptyServer()}, + {visitServer(), next.NewNetworkServiceServer(next.NewNetworkServiceServer(visitServer())), visitServer()}, + {visitServer(), next.NewNetworkServiceServer(next.NewNetworkServiceServer(emptyServer())), visitServer()}, } - expects := []int{1, 2, 3, 0, 1, 2, 1, 2, 3, 3, 1, 2} + expects := []int{1, 2, 3, 0, 1, 2, 1, 2, 3, 3, 1, 2, 3, 1} for i, sample := range servers { ctx := visit(context.Background()) s := next.NewNetworkServiceServer(sample...) From 51e26ef2478520695ac4788ac62fc3684ffc4c4f Mon Sep 17 00:00:00 2001 From: denis-tingajkin Date: Tue, 31 Mar 2020 13:35:43 +0700 Subject: [PATCH 2/5] fix format Signed-off-by: denis-tingajkin --- pkg/networkservice/core/next/client.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/networkservice/core/next/client.go b/pkg/networkservice/core/next/client.go index 7479e6918..af3d2dbd5 100644 --- a/pkg/networkservice/core/next/client.go +++ b/pkg/networkservice/core/next/client.go @@ -20,6 +20,7 @@ package next import ( "context" + "github.com/golang/protobuf/ptypes/empty" "google.golang.org/grpc" @@ -51,7 +52,7 @@ func NewNetworkServiceClient(clients ...networkservice.NetworkServiceClient) net if len(clients) == 0 { return &tailClient{} } - return NewWrappedNetworkServiceClient(defaultWrapper, clients...) + return NewWrappedNetworkServiceClient(notWrapClient, clients...) } func (n *nextClient) Request(ctx context.Context, request *networkservice.NetworkServiceRequest, opts ...grpc.CallOption) (*networkservice.Connection, error) { @@ -68,7 +69,7 @@ func (n *nextClient) Close(ctx context.Context, conn *networkservice.Connection, return n.clients[n.index].Close(withNextClient(ctx, nil), conn, opts...) } -func defaultWrapper(c networkservice.NetworkServiceClient) networkservice.NetworkServiceClient { +func notWrapClient(c networkservice.NetworkServiceClient) networkservice.NetworkServiceClient { return c } From 4b6037707fdeaacc53d649514c838b429b01c6ab Mon Sep 17 00:00:00 2001 From: denis-tingajkin Date: Wed, 1 Apr 2020 01:39:34 +0700 Subject: [PATCH 3/5] review comments Signed-off-by: denis-tingajkin --- pkg/networkservice/core/next/client.go | 43 +++++++++++++------------- pkg/networkservice/core/next/server.go | 43 +++++++++++++------------- 2 files changed, 42 insertions(+), 44 deletions(-) diff --git a/pkg/networkservice/core/next/client.go b/pkg/networkservice/core/next/client.go index af3d2dbd5..7f4dee16e 100644 --- a/pkg/networkservice/core/next/client.go +++ b/pkg/networkservice/core/next/client.go @@ -28,8 +28,9 @@ import ( ) type nextClient struct { - clients []networkservice.NetworkServiceClient - index int + clients []networkservice.NetworkServiceClient + index int + nextParent networkservice.NetworkServiceClient } // ClientWrapper - a function that wraps around a networkservice.NetworkServiceClient @@ -42,7 +43,7 @@ type ClientChainer func(...networkservice.NetworkServiceClient) networkservice.N func NewWrappedNetworkServiceClient(wrapper ClientWrapper, clients ...networkservice.NetworkServiceClient) networkservice.NetworkServiceClient { rv := &nextClient{} for _, c := range clients { - appendClient(rv, wrapper, c) + rv.clients = append(rv.clients, wrapper(c)) } return rv } @@ -52,33 +53,31 @@ func NewNetworkServiceClient(clients ...networkservice.NetworkServiceClient) net if len(clients) == 0 { return &tailClient{} } - return NewWrappedNetworkServiceClient(notWrapClient, clients...) + return NewWrappedNetworkServiceClient(func(client networkservice.NetworkServiceClient) networkservice.NetworkServiceClient { + return client + }, clients...) } func (n *nextClient) Request(ctx context.Context, request *networkservice.NetworkServiceRequest, opts ...grpc.CallOption) (*networkservice.Connection, error) { - if n.index+1 < len(n.clients) { - return n.clients[n.index].Request(withNextClient(ctx, &nextClient{clients: n.clients, index: n.index + 1}), request, opts...) + if n.index == 0 { + if nextParent := Client(ctx); nextParent != nil { + n.nextParent = nextParent + } } - return n.clients[n.index].Request(withNextClient(ctx, nil), request, opts...) -} - -func (n *nextClient) Close(ctx context.Context, conn *networkservice.Connection, opts ...grpc.CallOption) (*empty.Empty, error) { if n.index+1 < len(n.clients) { - return n.clients[n.index].Close(withNextClient(ctx, &nextClient{clients: n.clients, index: n.index + 1}), conn, opts...) + return n.clients[n.index].Request(withNextClient(ctx, &nextClient{nextParent: n.nextParent, clients: n.clients, index: n.index + 1}), request, opts...) } - return n.clients[n.index].Close(withNextClient(ctx, nil), conn, opts...) + return n.clients[n.index].Request(withNextClient(ctx, n.nextParent), request, opts...) } -func notWrapClient(c networkservice.NetworkServiceClient) networkservice.NetworkServiceClient { - return c -} - -func appendClient(next *nextClient, wrapper ClientWrapper, client networkservice.NetworkServiceClient) { - if v, ok := client.(*nextClient); ok { - for _, c := range v.clients { - appendClient(next, wrapper, c) +func (n *nextClient) Close(ctx context.Context, conn *networkservice.Connection, opts ...grpc.CallOption) (*empty.Empty, error) { + if n.index == 0 { + if nextParent := Client(ctx); nextParent != nil { + n.nextParent = nextParent } - return } - next.clients = append(next.clients, wrapper(client)) + if n.index+1 < len(n.clients) { + return n.clients[n.index].Close(withNextClient(ctx, &nextClient{nextParent: n.nextParent, clients: n.clients, index: n.index + 1}), conn, opts...) + } + return n.clients[n.index].Close(withNextClient(ctx, n.nextParent), conn, opts...) } diff --git a/pkg/networkservice/core/next/server.go b/pkg/networkservice/core/next/server.go index 3bc895d06..1de737ab4 100644 --- a/pkg/networkservice/core/next/server.go +++ b/pkg/networkservice/core/next/server.go @@ -27,8 +27,9 @@ import ( ) type nextServer struct { - servers []networkservice.NetworkServiceServer - index int + nextParent networkservice.NetworkServiceServer + servers []networkservice.NetworkServiceServer + index int } // ServerWrapper - A function that wraps a networkservice.NetworkServiceServer @@ -41,7 +42,7 @@ type ServerChainer func(...networkservice.NetworkServiceServer) networkservice.N func NewWrappedNetworkServiceServer(wrapper ServerWrapper, servers ...networkservice.NetworkServiceServer) networkservice.NetworkServiceServer { rv := &nextServer{} for _, s := range servers { - appendServer(rv, wrapper, s) + rv.servers = append(rv.servers, wrapper(s)) } return rv } @@ -52,33 +53,31 @@ func NewNetworkServiceServer(servers ...networkservice.NetworkServiceServer) net if len(servers) == 0 { return &tailServer{} } - return NewWrappedNetworkServiceServer(notWrapServer, servers...) + return NewWrappedNetworkServiceServer(func(server networkservice.NetworkServiceServer) networkservice.NetworkServiceServer { + return server + }, servers...) } func (n *nextServer) Request(ctx context.Context, request *networkservice.NetworkServiceRequest) (*networkservice.Connection, error) { - if n.index+1 < len(n.servers) { - return n.servers[n.index].Request(withNextServer(ctx, &nextServer{servers: n.servers, index: n.index + 1}), request) + if n.index == 0 { + if nextParent := Server(ctx); nextParent != nil { + n.nextParent = nextParent + } } - return n.servers[n.index].Request(withNextServer(ctx, nil), request) -} - -func (n *nextServer) Close(ctx context.Context, conn *networkservice.Connection) (*empty.Empty, error) { if n.index+1 < len(n.servers) { - return n.servers[n.index].Close(withNextServer(ctx, &nextServer{servers: n.servers, index: n.index + 1}), conn) + return n.servers[n.index].Request(withNextServer(ctx, &nextServer{nextParent: n.nextParent, servers: n.servers, index: n.index + 1}), request) } - return n.servers[n.index].Close(withNextServer(ctx, nil), conn) + return n.servers[n.index].Request(withNextServer(ctx, n.nextParent), request) } -func notWrapServer(server networkservice.NetworkServiceServer) networkservice.NetworkServiceServer { - return server -} - -func appendServer(next *nextServer, wrapper ServerWrapper, server networkservice.NetworkServiceServer) { - if v, ok := server.(*nextServer); ok { - for _, c := range v.servers { - appendServer(next, wrapper, c) +func (n *nextServer) Close(ctx context.Context, conn *networkservice.Connection) (*empty.Empty, error) { + if n.index == 0 { + if nextParent := Server(ctx); nextParent != nil { + n.nextParent = nextParent } - return } - next.servers = append(next.servers, wrapper(server)) + if n.index+1 < len(n.servers) { + return n.servers[n.index].Close(withNextServer(ctx, &nextServer{nextParent: n.nextParent, servers: n.servers, index: n.index + 1}), conn) + } + return n.servers[n.index].Close(withNextServer(ctx, n.nextParent), conn) } From 8f262c8155174e1494f1c10212822eb9c1a6d950 Mon Sep 17 00:00:00 2001 From: denis-tingajkin Date: Wed, 1 Apr 2020 01:45:38 +0700 Subject: [PATCH 4/5] fix nil context passing case Signed-off-by: denis-tingajkin --- pkg/networkservice/core/next/context.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/networkservice/core/next/context.go b/pkg/networkservice/core/next/context.go index a9baa71f6..bcc66932f 100644 --- a/pkg/networkservice/core/next/context.go +++ b/pkg/networkservice/core/next/context.go @@ -48,6 +48,9 @@ func withNextServer(parent context.Context, next networkservice.NetworkServiceSe // Server - // Returns the Server networkservice.NetworkServiceServer to be called in the chain from the context.Context func Server(ctx context.Context) networkservice.NetworkServiceServer { + if ctx == nil { + return nil + } rv, ok := ctx.Value(nextServerKey).(networkservice.NetworkServiceServer) if !ok { client, ok := ctx.Value(nextClientKey).(networkservice.NetworkServiceClient) @@ -74,6 +77,9 @@ func withNextClient(parent context.Context, next networkservice.NetworkServiceCl // Client - // Returns the Client networkservice.NetworkServiceClient to be called in the chain from the context.Context func Client(ctx context.Context) networkservice.NetworkServiceClient { + if ctx == nil { + return nil + } rv, ok := ctx.Value(nextClientKey).(networkservice.NetworkServiceClient) if !ok { server, ok := ctx.Value(nextServerKey).(networkservice.NetworkServiceServer) From 1d5588cc84d32b48e825132a8f5912d8c3102afd Mon Sep 17 00:00:00 2001 From: denis-tingajkin Date: Wed, 1 Apr 2020 16:29:48 +0700 Subject: [PATCH 5/5] self code review Signed-off-by: denis-tingajkin --- pkg/networkservice/core/next/client.go | 6 +++--- pkg/networkservice/core/next/context.go | 6 ------ pkg/networkservice/core/next/server.go | 6 +++--- 3 files changed, 6 insertions(+), 12 deletions(-) diff --git a/pkg/networkservice/core/next/client.go b/pkg/networkservice/core/next/client.go index 7f4dee16e..d07083c44 100644 --- a/pkg/networkservice/core/next/client.go +++ b/pkg/networkservice/core/next/client.go @@ -41,7 +41,7 @@ type ClientChainer func(...networkservice.NetworkServiceClient) networkservice.N // NewWrappedNetworkServiceClient chains together clients with wrapper wrapped around each one func NewWrappedNetworkServiceClient(wrapper ClientWrapper, clients ...networkservice.NetworkServiceClient) networkservice.NetworkServiceClient { - rv := &nextClient{} + rv := &nextClient{clients: make([]networkservice.NetworkServiceClient, 0, len(clients))} for _, c := range clients { rv.clients = append(rv.clients, wrapper(c)) } @@ -59,7 +59,7 @@ func NewNetworkServiceClient(clients ...networkservice.NetworkServiceClient) net } func (n *nextClient) Request(ctx context.Context, request *networkservice.NetworkServiceRequest, opts ...grpc.CallOption) (*networkservice.Connection, error) { - if n.index == 0 { + if n.index == 0 && ctx != nil { if nextParent := Client(ctx); nextParent != nil { n.nextParent = nextParent } @@ -71,7 +71,7 @@ func (n *nextClient) Request(ctx context.Context, request *networkservice.Networ } func (n *nextClient) Close(ctx context.Context, conn *networkservice.Connection, opts ...grpc.CallOption) (*empty.Empty, error) { - if n.index == 0 { + if n.index == 0 && ctx != nil { if nextParent := Client(ctx); nextParent != nil { n.nextParent = nextParent } diff --git a/pkg/networkservice/core/next/context.go b/pkg/networkservice/core/next/context.go index bcc66932f..a9baa71f6 100644 --- a/pkg/networkservice/core/next/context.go +++ b/pkg/networkservice/core/next/context.go @@ -48,9 +48,6 @@ func withNextServer(parent context.Context, next networkservice.NetworkServiceSe // Server - // Returns the Server networkservice.NetworkServiceServer to be called in the chain from the context.Context func Server(ctx context.Context) networkservice.NetworkServiceServer { - if ctx == nil { - return nil - } rv, ok := ctx.Value(nextServerKey).(networkservice.NetworkServiceServer) if !ok { client, ok := ctx.Value(nextClientKey).(networkservice.NetworkServiceClient) @@ -77,9 +74,6 @@ func withNextClient(parent context.Context, next networkservice.NetworkServiceCl // Client - // Returns the Client networkservice.NetworkServiceClient to be called in the chain from the context.Context func Client(ctx context.Context) networkservice.NetworkServiceClient { - if ctx == nil { - return nil - } rv, ok := ctx.Value(nextClientKey).(networkservice.NetworkServiceClient) if !ok { server, ok := ctx.Value(nextServerKey).(networkservice.NetworkServiceServer) diff --git a/pkg/networkservice/core/next/server.go b/pkg/networkservice/core/next/server.go index 1de737ab4..990d42629 100644 --- a/pkg/networkservice/core/next/server.go +++ b/pkg/networkservice/core/next/server.go @@ -40,7 +40,7 @@ type ServerChainer func(...networkservice.NetworkServiceServer) networkservice.N // NewWrappedNetworkServiceServer - chains together the servers provides with the wrapper wrapped around each one in turn. func NewWrappedNetworkServiceServer(wrapper ServerWrapper, servers ...networkservice.NetworkServiceServer) networkservice.NetworkServiceServer { - rv := &nextServer{} + rv := &nextServer{servers: make([]networkservice.NetworkServiceServer, 0, len(servers))} for _, s := range servers { rv.servers = append(rv.servers, wrapper(s)) } @@ -59,7 +59,7 @@ func NewNetworkServiceServer(servers ...networkservice.NetworkServiceServer) net } func (n *nextServer) Request(ctx context.Context, request *networkservice.NetworkServiceRequest) (*networkservice.Connection, error) { - if n.index == 0 { + if n.index == 0 && ctx != nil { if nextParent := Server(ctx); nextParent != nil { n.nextParent = nextParent } @@ -71,7 +71,7 @@ func (n *nextServer) Request(ctx context.Context, request *networkservice.Networ } func (n *nextServer) Close(ctx context.Context, conn *networkservice.Connection) (*empty.Empty, error) { - if n.index == 0 { + if n.index == 0 && ctx != nil { if nextParent := Server(ctx); nextParent != nil { n.nextParent = nextParent }