-
Notifications
You must be signed in to change notification settings - Fork 286
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
follow std http package Handler interface #237
Comments
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:
|
I can put out a diff for this during weekend. It is more about what direction we would like to go. Any thought? |
We discussed this at length, and were encouraged by the observability team to make it an explicit parameter. |
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 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. |
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. |
@yutongp for yarpc http is just a protocol. We have a separate uhttp module, which has nothing to do with yarpc whatsoever. |
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. |
@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. |
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 ***@***.***> 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
<#237 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAF7P3kfYyYsh75X4dPviSS5My5TYHl-ks5rbeqzgaJpZM4L9wG->
.
|
@sectioneight sure, no worries, no rush :D |
Just want open a discussion about:
vs
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:
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
hasContext
andWithContext
, user and std will usecontext
inrequest
. If we need to pick one source of truth, I think we should go withcontext
inrequest
.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.
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 acontext.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 :)
The text was updated successfully, but these errors were encountered: