RFC: fx.Context as a struct

[abhinav]

Following up our discussion yesterday: I would like to propose the following:

• fx.Context is a struct that embeds context.Context rather than an
  interface.
• UberFx values stored on fx.Context actually just use
  context.WithValue rather than wrapping a context.Context in a new
  type. Value accessors will be defined for all UberFx values.
• Corresponding methods will be declared on fx.Context which just call the
  accessor on itself.

Specifically,

package fx

type Context struct{ context.Context }

func (ctx Context) Logger() Logger {
    return GetLogger(ctx)
}

func GetLogger(ctx context.Context) Logger {
    // ..
}

This has a few advantages:

• Discoverability by having methods on fx.Context.

• Full compatibility with context.Context and WithValue. You won't lose
  UberFx information if you do,

  fxctx := fx.NewContext(ctx, host)
  ctx := fxctx.WithValue(...)

• Downgrading to context.Context and upgrading back to fx.Context just
  works.

  fxctx := fx.NewContext(ctx, host)
  var ctx2 context.Context = fxctx
  fxctx2 := fx.Context{ctx2}
  fmt.Println(fxctx.Logger() == fxctx2.Logger())  // true

The only thing to keep in mind is that accessors have to have reasonable
behavior for the case where a context.Context does not have UberFx
information on it.

Thoughts?

CC @breerly @kriskowal @willhug @sectioneight

[prashantv]

There may be performance implications to using a struct, since we'll be passing the struct by value, and when a by-value object is converted to an interface, it requires an allocation. Since the fx.Context will be passed to methods that expect a context.Context interface, it's possible that each of these will require an extra allocation to do the interface conversion.

We should ensure there's no performance hit if we want to do this.

[ascandella]

cc @anuptalwalkar who is doing the implementation

[ascandella]

And yes, the interface conversion was the primary motivation for us using an interface in he first place.

[abhinav]

If this affects perf negatively, an interface can still be used to get
context.Context compatibility provided that UberFx maintains control of all
implementations by adding a private method to the interface and providing a
public function to upgrade a context.Context back to an fx.Context.

package fx

type Context interface {
    context.Context
    
    // unexported function to ensure only this package implements this
    // interface.
    fx()
    
    // ...
}

func Upgrade(context.Context) Context {  // Better name TBD
    return &context{...}
}

func NewContext(context.Context, Host) Context {
    // ...
}

type context struct {
    // ..
}

func (c *context) fx() {}

func (c *context) Logger() Logger {
    return GetLogger(c.Context)
}

This gets rid of the fx.Context -> context.Context alloc.

[ascandella]

FWIW I was imagining Logger() and friends would be impure (e.g. have side effects of caching the return value)

…

On Wed, Dec 14, 2016 at 11:26 AM Abhinav Gupta ***@***.***> wrote: If this affects perf negatively, an interface can still be used to get context.Context compatibility provided that UberFx maintains control of all implementations by adding a private method to the interface and providing a public function to upgrade a context.Context back to an fx.Context. package fx type Context interface { context.Context // unexported function to ensure only this package implements this // interface. fx() // ... } func Upgrade(context.Context) Context { // Better name TBD return &context{...} } func NewContext(context.Context, Host) Context { // ... } type context struct { // .. } func (c *context) fx() {} func (c *context) Logger() Logger { return GetLogger(c.Context) } This gets rid of the fx.Context -> context.Context alloc. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#126 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAF7P4KCgTYQ9TQNuUlfXV5IQEWQGQNJks5rIELTgaJpZM4LNTGE> .

[prashantv]

Private method on the interface is a good idea. I'd avoid adding the Upgrade -- my understanding is that this shouldn't be needed, and the uberfx team actively wants to avoid wrapping.

[anuptalwalkar]

I like the private method idea to keep the implementation restricted.
Is there a performance impact on doing fxCtx := ctx.(fx.Context) ?

[abhinav]

@anuptalwalkar ctx.(fx.Context) will not work if context.WithValue was
used on an fx.Context.

fxctx := fx.NewContext(...)
ctx := context.WithValue(fxctx, ...)

fxctx2 := ctx.(fx.Context)  // this will panic

The two solutions recommended here would solve that.

@prashantv The upgrade function is needed to solve the problem mentioned
above. On second thought, it doesn't have to be public, but we definitely need
it.

Imagine that an UberFx InboundMiddleware adds UberFx information to incoming
contexts:

func (m *ctxMiddleware) Handle(
    ctx context.Context, ..., h transport.UnaryHandler,
) error {
    fxctx := fx.NewContext(ctx, m.Host)
    return h.Handle(fxctx, ...)
}

We get this context as a context.Context at the encoding layer. We need to
convert this back to an fx.Context to call the user handler. This would be
done with UberFx's Thrift function wrapper.

func WrapUnary(h fxthrift.UnaryHandlerFunc) yarpcthrift.UnaryHandler {
    return func(ctx context.Context, ...) error {
       fxctx := upgrade(ctx)
       return h(fxctx, ...)
    }
}

[anuptalwalkar]

I am following the same approach in the refactoring. Well, ctx.(fx.Context) works in WrapUnary since it is injected in the inboundMiddleware (unless we write a middleware to do WithValue in between the calls), but in this scenario upgrade works just as well. We definitely don't want to do unnecessary wrapping.

[abhinav]

ctx.(fx.Context) won't work if anything between the UberFx middleware and
WrapUnary calls context.WithValue on the context because the returned ctx
will not have the added fx.Context functions anymore.

[ascandella]

Let's go with Abhinavs suggestion of pure functions and structs and write some benchmarks. It seems cleanest.

…

On Wed, Dec 14, 2016 at 2:36 PM Abhinav Gupta ***@***.***> wrote: ctx.(fx.Context) won't work if anything between the UberFx middleware and WrapUnary calls context.WithValue on the context because the returned ctx will not have the added fx.Context functions anymore. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#126 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAF7P7z_dipuKfWLDmbZRRIafUkjKdGvks5rIG9ugaJpZM4LNTGE> .

[anuptalwalkar]

Added benchmarks for Context conversion - #131