x/tools: release static analysis tool for context propogation #16742
Labels
Milestone
Comments
|
Also @alandonovan for the status of the tooling |
|
There's a pull request at golang/lint#227 that tries to add this check to |
|
A tool that determines whether contexts are propagated correctly (instead of being forgotten and dropped) would also be useful in this situation, see google/go-github#529 (comment). |
dmitshur
added a commit
to google/go-github
that referenced
this issue
Feb 20, 2017
This reverts commit 6b55015. I've described full rationale for doing so in my comment at #526 (comment). I'll paste it here for convenience. Today, I experimented with updating some code I had that uses go-github to the new API and see if that would lead to any insight. For a good typical example, see https://github.com/shurcooL/notifications/pull/1/files. That perspective led me to better understanding, and I now think that the better thing to do is to revert commit 6b55015 and use the following signature for Do after all: Do(ctx context.Context, req *http.Request, v interface{}) Here are 4 reasons that make up the rationale/motivation for doing that: 1. First reason. After I switched to the new context branch of go-github locally and ran tests on my Go code that uses go-github, the compiler gave me very clear and actionable errors such as: # github.com/shurcooL/notifications/githubapi githubapi/githubapi.go:44: not enough arguments in call to s.clNoCache.Activity.ListNotifications have (nil) want (context.Context, *github.NotificationListOptions) githubapi/githubapi.go:53: not enough arguments in call to s.clNoCache.Activity.ListRepositoryNotifications have (string, string, nil) want (context.Context, string, string, *github.NotificationListOptions) githubapi/githubapi.go:151: not enough arguments in call to s.cl.Activity.ListRepositoryNotifications have (string, string, nil) want (context.Context, string, string, *github.NotificationListOptions) githubapi/githubapi.go:179: not enough arguments in call to s.cl.Activity.MarkThreadRead have (string) want (context.Context, string) githubapi/githubapi.go:193: not enough arguments in call to s.cl.Activity.MarkRepositoryNotificationsRead have (string, string, time.Time) want (context.Context, string, string, time.Time) FAIL github.com/shurcooL/notifications/githubapi [build failed] It was very helpful and very easy to update my code to pass the additional context parameter. I had a great time doing it. However, I only later noticed that I forgot about the calls to Do method that some of my code made. That code continued to compile without any errors, and while it worked, the context wasn't propagated. Spotting that was hard, and worst of all, it felt very inconsistent. When every single method has an extra parameter, which makes the compiler help you catch instances of code you need to update, why doesn't that also apply to calls to Do method? Even after I updated my calls to Do to pass context with req.WithContext(ctx), it was harder to be confident I hadn't missed some cases. For example, imagine a situation where you set the context on a request earlier, and then call Do(req, ...). In the end, having an explicit first parameter for passing context is a lot easier to see that context is being propagated. 2. Second reason. I believe my rationale in commit 6b55015 is not valid. It definitely shouldn't carry as much weight as I originally thought. The situation I described where req is created in another function and then passed in... That just doesn't seem like something that would happen often. It seems that code that creates a NewRequest and then does Do is more likely. E.g., something like https://github.com/shurcooL/notifications/blob/a03ac7eff1ecb7f92f0d91e32674137cc017286c/githubapi/githubapi.go#L248-L256. 3. Third reason. I originally noticed that between the two options of passing context separately and explicitly, and passing it via request, the latter was more in line with Go standard library. Clearly, the NewRequest and Do methods are modeled after same ones in net/http package: https://godoc.org/net/http#NewRequest https://godoc.org/net/http#Client.Do However, that's not evidence that it's the best way to pass context to Do. The net/http package API is frozen in Go1 and it couldn't have changed. A better place to look would be golang/go#11904, a proposal to make a friendlier context-aware http.Do which was resolved by creating the golang.org/x/net/context/ctxhttp package. Look at its Do method: // Do sends an HTTP request with the provided http.Client and returns // an HTTP response. // // If the client is nil, http.DefaultClient is used. // // The provided ctx must be non-nil. If it is canceled or times out, // ctx.Err() will be returned. func Do(ctx context.Context, client *http.Client, req *http.Request) (*http.Response, error) { if client == nil { client = http.DefaultClient } resp, err := client.Do(req.WithContext(ctx)) // If we got an error, and the context has been canceled, // the context's error is probably more useful. if err != nil { select { case <-ctx.Done(): err = ctx.Err() default: } } return resp, err } That was the outcome when the Go1 API freeze did not constrain the choice of Do's signature. 4. Fourth reason. There is a proposal golang/go#16742 that tracks the creation of a tool to help with validating correct context propagation. Such a tool is hinted at in context package documentation, but so far does not exist. Had it existed and if it were trivial to verify that context is propagated and not accidentally dropped, this reason would not be included. The fact is that it's much easier for users to validate correct propagation of context if the signature of Do is changed to accept ctx context.Context explicitly as first parameter. So, this is additional evidence I believe we should go with what's friendlier to users of the API. As motivated by first reason, I believe it's friendlier to break the API of Do method than it is not to break it (somewhat counter-intuitively). For these reasons, I think we should make the change of Do to: Do(ctx context.Context, req *http.Request, v interface{}) Which this revert does.
|
Any news on the hypothetical static analysis tool that the This is proving to be a source of much confusion; any feelings on updating the |
|
I'm sorry to report that there is no tool. The author of that intriguing comment had a tendency to use the present tense when describing uncertain future events. ;) |
|
In that case, I'll close this out. Thanks for the follow-up. |
|
Change https://golang.org/cl/130876 mentions this issue: |
gopherbot
pushed a commit
that referenced
this issue
Aug 22, 2018
This comment has been the source of much confusion and broken dreams. We can add it back if a tool ever gets released. Updates #16742 Change-Id: I4b9c179b7c60274e6ff1bcb607b82029dd9a893f Reviewed-on: https://go-review.googlesource.com/130876 Reviewed-by: Matt Layher <[email protected]> Reviewed-by: Alan Donovan <[email protected]> Reviewed-by: Brad Fitzpatrick <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Now that
contextis part of the standard library in Go 1.7, it would be nice for the tool mentioned in the comment ofcontext.TODO()to be available to the public: https://golang.org/pkg/context/#TODO.If this isn't feasible for some reason, I instead propose that this comment be removed, to prevent misleading others who are looking for the same tool:
/cc @broady
The text was updated successfully, but these errors were encountered: