Move service handler creation after FX creates dispatcher

[ghost]

Because we create service before the yarpc.Dispatcher it is hard for clients to get it in the constructor, they have to get it during request serving to e.g. create client.

With this change services will be able to resolve Dispatcher at construction time.

[mention-bot]

@alsamylkin, thanks for your PR! By analyzing the history of the files in this pull request, we identified @shawnburke, @sectioneight and @dmcaulay to be potential reviewers.

[coveralls]

Coverage increased (+0.2%) to 92.199% when pulling 3cab60b on exposeDispatcher into 8622d3c on master.

[HelloGrayson]

This approach appears to be too limiting.

• Users need to be able to configure clients, and then inject them across multiple handlers. Injecting the dispatcher at this way will lead to instantiating the same clients over again for many handlers.
• Clients should ideally all be configured in the same place
• Beyond clients, there needs to be a way to inject other shared deps into handlers

[HelloGrayson]

What exactly does CreateThriftServiceFunc provide? Why not just allow the user to construct their own handler in main using dig?

[ghost]

This approach doesn't limit the existing functionality, it moves the dispatcher from DIG to the handler constructor, so users don't have to resolve it.

Before we asked a user to create transport.Procedures and give them to us to attach the dispatcher,
but

1. They may need a dispatcher to create clients
2. We don't do anything special about it now, just call dispatcher.Register()

[ghost]

Another thing: right now handler constructor(CreateThriftServiceFunc) is called before dispatcher is even created, which makes common client initialization impossible. E.g. this is what users need to do now:

func NewYarpcThriftHandler(_ service.Host) error {
        var d *yarpc.Dispatcher
        if err := dig.Resolve(&d); err != nil {
            return err
        }
 
        handler := &YarpcHandler{
		items: map[string]string{},
		client : keyvalueclient.New(d),
	}

	return kvs.New(handler)
}

with this change, it will be something like:

func NewYarpcThriftHandler(_ service.Host, d *yarpc.Dispatcher) error {
        handler := &YarpcHandler{
		items: map[string]string{},
		client : keyvalueclient.New(d),
	}

	dispatcher.Register(kvs.New(handler))
	return nil
}

[ghost]

Actually it can be little better:

func NewYarpcThriftHandler(_ service.Host, d *yarpc.Dispatcher) ([]transport.Procedure, error) {
        handler := &YarpcHandler{
		items:  map[string]string{},
		client: keyvalueclient.New(d),
	}

	return kvs.New(handler), nil
}

[coveralls]

Coverage increased (+0.2%) to 92.209% when pulling c6202a4 on exposeDispatcher into 8622d3c on master.

[HelloGrayson]

So how would I share the client between 2 handlers? What if I have a separate shape in my application, not a handler, that also needs the same client?

It seems to me like there needs to be a way to configure and register clients in DIG, so that all shapes, including handlers, can simply declare their dependencies on said client.

[ghost]

I am not sure, if they should be easily shared, but may be it can be autogenerated and cached?
Something like:

func Get() keyvalueclient.Interface {
   if cached {
      return client
   }
   var d *yarpc.Dispatcher
   if err := dig.Resolve(&d); err != nil {
      return nil
   }
   cached = true
   client = keyvalueclient.New(d)
   return client
}

Although, some people would argue, that this is an extra function call and DIG doesn't yet support multiple objects of the same type..

[HelloGrayson]

The problem we are talking about is the problem dig is supposed to solve. Users need to be able to register shapes with dig, either programmatically or using config, and then trust dig to build the graph correctly.

I don't think this part should behave differently - register client into graph, register handlers into graph, every object declares its deps which dig resolves.

Using dig will provide a single way of solving this for all usecases, not just this client+handler one.

[HelloGrayson]

If possible, I'd like to get @glibsm in here for a proposal:

The proposal is that all deps/shapes in a service (the application graph), including clients and handlers, get registered into a dig so that the full graph can be resolved.

This has the effect of cleaning a few things up:

• No need to inject dig's graph anywhere (this is really important)
• No need to inject the dispatcher anywhere (equally important)
• Users simply register shapes and declare deps, letting dig resolve

I worry that by allowing the graph or dispatcher to be used, users will actually be shooting themselves in the foot. We should be making it easy to do the right thing, instead of handing them a loaded gun (excuse the analogy).

Crazy?

[ghost]

I do like where we are going with it and I think it is time to set up a meeting in person and discuss everything offline.

[dmcaulay]

I think I like this... Are you proposing that we keep the graph private and only register/resolve internally?

I'm thinking about doing this in Glue because I don't want it to be too easy to breakdown the layers of abstraction that we're trying to build. I was even thinking about having different graphs for each layer. Still brainstorming though...

[dmcaulay]

All layers would still need access to the fx graph though. Not quite sure how that would work.

[dmcaulay]

DIG and Glue are solving this problem very differently.

Problem- Too much boilerplate needed to wire up your service.

Glue- If you follow these patterns and each layer only depends on x, y, z, then we'll generate some simple code to wire it up for you.

DIG- Register all the things and depend on whatever you want (no cycles) and we'll wire it up for you. This is really cool, but it makes me nervous.

I'm obviously biased 😃

[coveralls]

Coverage increased (+0.1%) to 92.146% when pulling 002778a on exposeDispatcher into 8622d3c on master.

[coveralls]

Coverage increased (+0.1%) to 92.146% when pulling 002778a on exposeDispatcher into 8622d3c on master.

[HelloGrayson]

Ok, I wont block this PR anymore.

After talking with the UberFX team, @glibsm and I are going to design an alternate API that takes a DI-first approach. That design and prototype will inform what changes will have to be made to the existing experience.

For now, I count this as an improvement to the existing experience, in that now clients can be instantiated slightly before the hot path. It doesn't solve the DI problem in a generic way, but that is ok and will be solved by the plan @glibsm and I coming up with.

[HelloGrayson]

lgtm once Travis is happy.

[coveralls]

Coverage increased (+0.1%) to 92.296% when pulling 9adaa9e on exposeDispatcher into 62a9f89 on master.

[coveralls]

Coverage increased (+0.1%) to 92.296% when pulling 9adaa9e on exposeDispatcher into 62a9f89 on master.