Add the possible to make chain of chains via pkg/next

[denis-tingaikin]

Signed-off-by: denis-tingajkin denis.tingajkin@xored.com

Motivation

#172

[edwarnicke]

I'm not following the problem we are trying to solve here... could you say more?

It's unclear to me why the existing code does not allow you to use one chain as an element in another.

[denis-tingaikin]

@edwarnicke

could you say more?

Sure, we are trying to use chains inside chains. For example:

next.NewServer(next.NetServer(somepkg.NewServer()), somepkg.NewServer())

It's unclear to me why the existing code does not allow you to use one chain as an element in another.

The problem related to two things:

1. tailServer, tailClient elements are not using next.Client()/next.Servers() (the chains brokes here)
2. Inner chain elements rewrites value ofnextServer/nextClient

[edwarnicke]

Oh... that is nicely subtle. Good catch.

Question thought... might it not be simpler and clearer to simply do something like replacing

https://github.com/networkservicemesh/sdk/blob/master/pkg/networkservice/core/next/server.go#L64

return n.servers[n.index].Request(withNextServer(ctx, nil), request)

with

return n.servers[n.index].Request(ctx, request)

That way any 'nextServer' coming from the parent would show up for the child?

[denis-tingaikin]

@edwarnicke good idea. Thanks, fixed.

[edwarnicke]

Question... why return nil here instead of tailServer?

[denis-tingaikin]

actually it can be nil in two cases:

1. At the first call of method request/close with ctx = nil. In this case, we looking for the next parent chain and value can be nil
2. Call this method outside of the chain. I.e. user not used next.NewServer() and passed nil context. Not sure that we should return tailServer in this case

[denis-tingaikin]

Finally, I think it will be better just throw nil panic exception for the case №2.
Fixed by 1d5588c

[edwarnicke]

Why would we panic if called outside of a chain?

[denis-tingaikin]

Why would we panic if called outside of a chain?

I think ignoring errors is not a good pattern for this place.

1. We already use similar behavior:
   1.1 https://github.com/networkservicemesh/sdk/blob/master/pkg/networkservice/core/trace/context.go#L43
   1.2. https://github.com/networkservicemesh/sdk-vppagent/blob/master/pkg/networkservice/vppagent/context.go#L50
   1.3. https://github.com/networkservicemesh/sdk/blob/master/pkg/networkservice/common/clienturl/context.go#L44
   1.4. https://github.com/networkservicemesh/sdk/blob/master/pkg/networkservice/common/discover/context.go#L46
   1.5. https://github.com/networkservicemesh/sdk/blob/master/pkg/registry/core/trace/context.go#L43
   ...
2. I think if the user tries to use nil context to get the value then it is an invalid case. And we should not ignore the invalid state.

[edwarnicke]

That's fair :)