follow std http package Handler interface

[yutongp]

Just want open a discussion about:

 // Handler is a context-aware extension of http.Handler.
type Handler interface {
    ServeHTTP(ctx context.Context, w http.ResponseWriter, r *http.Request)
}

vs

type Handler interface {
        ServeHTTP(ResponseWriter, *Request)
}

I am playing with std http with context recently and I found myself adopt easily. Here are reasons why I think following std defined interface is better:

1. context in one place. you always get context out of a request every time you use it and put context back every time you update it. if you look at uhttp/filters.go,

   func (f contextFilter) Apply(ctx context.Context, w http.ResponseWriter, r *http.Request, next Handler) {
   ctx = fx.NewContext(ctx, f.host)
   next.ServeHTTP(ctx, w, r)
   }

   contextFilter failed to set context in request. and at https://github.com/uber-go/fx/blob/master/modules/uhttp/filters.go#L80, tracingFilter overwrite request context directly instead wrapping original request context. Since context is already in request, there is no need to pass context again and have the risk to make these two context out of sync. Since request has Context and WithContext, user and std will use context in request. If we need to pick one source of truth, I think we should go with context in request.

2. no std http.Handler wrapper. 100% compatible with any http related package out there including std right away and always. I think this is important as well. When I select package, I will try to use ones follow std since std API appear to be stable and rarely change.

3. no "golang.org/x/net/context" incompatible issue. a handler implemented: ServeHTTP(ctx context.Context, w http.ResponseWriter, r *http.Request) can not be used as a

   type Handler interface {
       ServeHTTP(ctx golang.org/x/net/context.Context, w http.ResponseWriter, r *http.Request)
   }

   context.Context can be passed as golnag.org/x/next/context.Context, but these two handler interface are not compatible. go1.8 has go fix which aiming to "solve" this issue, but following std interface will let us move a way from this mess.

Those are why I think we should use std http handler interface. Let's discuss :)

[yutongp]

BTW I do agree context should be the first param in all business logic layers passed HTTP layer. So the user define HTTP handler probably looks like:

func (h Handler) ServeHTTP(w ResponseWriter, r *Request) {
  ctx := r.Context() 
  businessReq := toBusinessModel(r)
  businessResp := handleBusiness(ctx , businessReq)
  ....
}

[yutongp]

I can put out a diff for this during weekend. It is more about what direction we would like to go. Any thought?

[ascandella]

We discussed this at length, and were encouraged by the observability team to make it an explicit parameter.

cc @anuptalwalkar @yurishkuro

[yurishkuro]

This may sound like a nuanced answer, but "when in Rome..." In other words, having a library that deviates from std lib conventions can hurt its chances of adoption. I am a big fan of passing the context explicitly, especially when designing business service APIs, because it encourages developers to pass it around and make use of it (e.g. respect cancellation signals). But given that std lib chose a different approach for http.Request, I would make an exception and stick with the std lib API. @yutongp's dual source of truth argument makes a lot of sense.

On a separate note, I thought Fx would be integrated with yarpc and this whole question would become a moot point, since people won't be writing http handlers explicitly, but rather yarpc handlers that do pass context as argument.

[yutongp]

I feel like yarpc doesn't have all the features for a pure Go HTTP service (why pure HTTP service? that could be another discussion) for now. Just leak of supporting url-encoded data and user unable to set http response code is almost a No for a lot of use cases. So for now having a http package with all the tool chains we need make sense I think.

[glibsm]

@yutongp for yarpc http is just a protocol. We have a separate uhttp module, which has nothing to do with yarpc whatsoever.

[ghost]

I think @yutongp caught us in the transition state: we just removed a custom fx.context and haven't fully moved to use the standard one yet.

[yutongp]

@alsamylkin I like you guys changed fx to use std context. When I saw that commit, I said 'YES!' to myself. I think std context is definitely the direction to go. appengine used to have custom context and they moved away.

[ascandella]

BTW I fully support this change. Just want to get a chance to review it. On Sat, Feb 11, 2017 at 9:07 AM Yutong Pei notifications@github.com wrote:

@alsamylkin https://github.com/alsamylkin I like you guys changed fx to use std context. When I saw that commit, I said 'YES!' to myself. I think std context is definitely the direction to go. appengine used to have custom context and they moved away.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/uber-go/fx/issues/237#issuecomment-279160095, or mute the thread https://github.com/notifications/unsubscribe-auth/AAF7P3kfYyYsh75X4dPviSS5My5TYHl-ks5rbeqzgaJpZM4L9wG- .

[yutongp]

@sectioneight sure, no worries, no rush :D

[yutongp]

239