xds: HTTP filters on the client ignore the done callback

[easwars]

The xDS resolver currently implements an interceptorList struct that chains all the HTTP filters on the client and runs them one after the other. See:

grpc-go/internal/xds/resolver/serviceconfig.go

Line 364 in 19e4128

type interceptorList struct {

When a new config selector is created by the xDS resolver, it creates an interceptorList for every weighted cluster within every route. See:

grpc-go/internal/xds/resolver/xds_resolver.go

Line 390 in 19e4128

interceptor, err := newInterceptor(r.xdsConfig.Listener.APIListener.HTTPFilters, wc.HTTPFilterConfigOverride, rt.HTTPFilterConfigOverride, r.xdsConfig.VirtualHost.HTTPFilterConfigOverride)

When the config selector is asked to return matching configuration for an RPC, it returns this interceptorList as part of the RPCConfig. See:

grpc-go/internal/xds/resolver/serviceconfig.go

Line 218 in 19e4128

Interceptor: cluster.interceptor,

When an RPC is initiated and a new stream is being created for it, the config selector is invoked and the interceptors are run here:

grpc-go/stream.go

Line 273 in 19e4128

cs, err := rpcConfig.Interceptor.NewStream(ctx, rpcInfo, done, ns)

But the NewStream method on the interceptorList ignores the done callback passed to it, and instead passes an empty function to the chain of interceptors. See:

grpc-go/internal/xds/resolver/serviceconfig.go

Line 376 in 19e4128

return newStream(ctx, func() {})

Also, newClientStream passes an empty function as the done callback as well here:

grpc-go/stream.go

Line 282 in 19e4128

return newStream(ctx, func() {})

@dfawley

• Should the interceptorList ignore the done callback?
• Should newClientStream pass an empty function as the done callback?
• With the filter state retention changes that we are making as part of A83, when a Filter instance shares some state to all of its interceptors, it would have to know exactly when the interceptors are done. Filters that share state could still wrap the the done callback passed to them in NewStream and do whatever they have to do when the interceptor is done.

[dfawley]

Unless I'm missing something, newClientStream and interceptorList are like the root of the callback chain. They don't currently need a notification for themselves, so they start things with an empty callback. That callback is wrapped by the interceptors (or forwarded if not needed).

But the NewStream method on the interceptorList ignores the done callback passed to it, and instead passes an empty function to the chain of interceptors. See:

I don't see where interceptorList.NewStream accepts a done callback. It builds a newStream closure that accepts and forwards a done callback, but that's it.

[easwars]

Let's add a comment in the code to ensure future readers have an easier time.