Skip to content
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

x/tools: release static analysis tool for context propogation #16742

Closed
mdlayher opened this issue Aug 16, 2016 · 7 comments
Closed

x/tools: release static analysis tool for context propogation #16742

mdlayher opened this issue Aug 16, 2016 · 7 comments

Comments

@mdlayher
Copy link
Member

Now that context is part of the standard library in Go 1.7, it would be nice for the tool mentioned in the comment of context.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:

TODO is recognized by static analysis tools that determine whether Contexts are propagated correctly in a program.

/cc @broady

@dgryski
Copy link
Contributor

dgryski commented Aug 16, 2016

Also @alandonovan for the status of the tooling

@dsnet
Copy link
Member

dsnet commented Aug 16, 2016

There's a pull request at golang/lint#227 that tries to add this check to lint, but the check really should be done in vet or in some other tool.

@dmitshur
Copy link
Contributor

dmitshur commented Feb 18, 2017

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.
@shazow
Copy link
Contributor

shazow commented Aug 22, 2018

Any news on the hypothetical static analysis tool that the context.TODO() comment refers to?

This is proving to be a source of much confusion; any feelings on updating the context.TODO() comment meanwhile?

@alandonovan
Copy link
Contributor

alandonovan commented Aug 22, 2018

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. ;)

@mdlayher
Copy link
Member Author

In that case, I'll close this out. Thanks for the follow-up.

@gopherbot
Copy link

Change https://golang.org/cl/130876 mentions this issue: context: don't talk about tools that don't exist

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 <mdlayher@gmail.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Aug 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

8 participants