L90: Go: Support grpc.Dial interceptors

[anicr7]

No description provided.

[dfawley]

+ gRPC-Go team as FYI / to review if interested.

[dfawley]

I think more discussion is needed here.

1. Do we support multiple interceptors? I.e. if I register an interceptor but one has already been registered, will both be called?
2. What about races? Do we need to worry about locks/atomics here, or do we restrict it so this must only be called during init functions? I believe this implies grpc.Dial may never be called in a goroutine forked in an init, which is a bit hard to explain.

[anicr7]

1. Supporting multiple interceptors in clientconn seems tricky. As we have a callback to grpc.dialContext in the vars, if we do allow multiple interceptors it is not clear how an interceptor can figure out if it is the last one in the chain and can call grpc.dialContext.

It is definitely possible to support this using another indirection layer(either in the application or in gRPC package)

type DialInterceptors interface {
	Intercept(ctx context.Context, target string, opts ...DialOption) (context.Context, string, []DialOption)
}

type MyInterceptor struct {
	interceptors []DialInterceptors
}

func (m *MyInterceptor) Dial(ctx context.Context, target string, defaultDial dialerCallback, opts ...DialOption) (*ClientConn, error) {
	for _, i := range m.interceptors {
		ctx, target, opts = i.Intercept(ctx, target, opts...)
	}
	return defaultDial(ctx, target, opts...)
}

This makes it possible to support multiple interceptors but also indicates that the ClientConn can only be established by the default dialer. The interceptors can only modify ctx, target, opts. Would this approach be preferable?

2. My initial idea here was to restrict the Set so that it can only be called during init(). I am also ok supporting it outside init() through locks/atomics.

We can support setting the dial interceptor through atomic.LoadPointer and atomic.StorePointer to avoid locks. An example here:

var dialInterceptor unsafe.Pointer

func SetDialInterceptor(interceptor DialInterceptor) {
	atomic.StorePointer(&dialInterceptor, unsafe.Pointer(&interceptor))
}

func DialContext(ctx context.Context, target string, opts ...DialOption) (conn *ClientConn, err error) {
	di := atomic.LoadPointer(&dialInterceptor)
	if di == nil {
		return dialContext(ctx, target, opts...)
	}

	return (*(*DialInterceptor)(di))(ctx, target, dialContext, opts...)
}

Though even if we do support atomics, there is always going to be a race condition between an init() which calls both SetDialInterceptor and grpc.Dial in another go routine.

I believe this implies grpc.Dial may never be called in a goroutine forked in an init, which is a bit hard to explain.

I don't think I completely understand the use case here. Calling grpc.Dial in init() should be fine with or without a go routine. But the race condition for the Set vs Dial might exist if they are executed in different go routines. The behavior of the grpc.Dial in the go routine will depend on whether Set was called earlier or not.

Is there a way we can prevent this race condition?

[menghanl]

• I think it's OK to support one interceptor now.
• Only allow setting interceptor in init() sounds good.
  • If a grpc.Dial() is called in init(), there doesn't seem to be any good solution to it. If the interceptor is not added by this or another init(), it's also too late for this dial.