all: audit context.TODO usage

[nvanbenschoten]

This change audits our use of context.TODO, bringing down the usage count from 634 to 302.

It does so with three changes:

• Using context.Background in all tests, as suggested by the the godocs. This is the pattern we should use for now on, and we may even want to add a style check to enforce it (@tamird?) Note, this step was reverted because of too much disagreement
• Using available contexts instead of context.TODO in a few places where a context was already available. I assume these were just missed during refactorings.
• Using context.Background in cobra.Cmd functions, because these are essentially main functions.

---

This change is 

[andreimatei]

As I was saying in https://paper.dropbox.com/doc/context.Context-and-how-it-relates-to-logging-and-tracing-in-CockroachDB-bFMpKpbR1vqnn6qmIBKTC?t=259843270295767#:t=259843270295767,
I don't think we should use context.Background() in tests; I think TODO is slightly better (assuming we only consider these 2 options). Our tests launch servers with true background work, and the test is very much the foreground; we should aim to highlight operations performed directly by the test, not munge them with the server's background work.

[tamird]

Perhaps we should have a factory method in testutils that takes a testing.TB and returns a context:

func NewContext(tb testing.TB) {
  return log.WithLogTagStr(context.Background(), "t", tb.Name())

Absolutely +1 on adding a style check.

(did not review 1st commit, otherwise LGTM).

---

Reviewed 61 of 61 files at r1, 7 of 7 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

pkg/sql/parser/eval.go, line 2487 at r2 (raw file):

// Eval implements the TypedExpr interface.
func (expr DefaultVal) Eval(ctx *EvalContext) (Datum, error) {
	log.Errorf(ctx.Ctx(), "unhandled type %T passed to Eval", expr)

what is with all this error logging? seems like it should just be removed.

---

Comments from Reviewable

[nvanbenschoten]

Why do you say that we shouldn't use context.Background in tests? It is what is recommended in the documentation for the context package, and it is what both Go and gRPC do. As the Go blog says: "Background is the root of any Context tree". context.TODO meanwhile is meant for locations where a context is not yet available, and is separated in part to allow for the development of static analysis tools and to serve as an indication of future work.

Our tests launch servers with true background work, and the test is very much the foreground

doesn't make much sense to me. context.Background is meant to be used in main functions and incoming requests, which are both very much handled in the foreground. Perhaps it should have been named context.Root or something, but we shouldn't let the confusing name trick us into using it incorrectly.

Interestingly, there was a proposal to add a T.Context() method to the testing package (for convenient cancellation on test completion), but that idea was subsequently abandoned in favor of sticking with ctx, cancel := context.WithCancel(context.Background()) in tests.

[tamird]

How about the test-named context suggestion?

…

On Mar 13, 2017 22:52, "Nathan VanBenschoten" ***@***.***> wrote: Why do you say that we shouldn't use context.Background in tests? It is what is recommended in the documentation for the context package, and it is what both Go <https://github.com/golang/go/blob/3792db518327c685da17ca6c6faa4e1d2da4c33c/src/net/dial_test.go#L370> and gRPC <https://github.com/grpc/grpc-go/blob/71d2ea4f75286a63b606aca2422cd17ff37fd5b8/balancer_test.go#L121> do. As the Go blog says <https://blog.golang.org/context#TOC_2.1.>: "Background is the root of any Context tree". context.TODO meanwhile is meant for locations where a context is not yet available, and is separated in part to allow for the development of static analysis tools and to serve as an indication of future work. Our tests launch servers with true background work, and the test is very much the foreground doesn't make much sense to me. context.Background is meant to be used in main functions and incoming requests, which are both very much handled in the foreground. Perhaps it should have been named context.Root or something, but we shouldn't let the confusing name trick us into using it incorrectly. Interestingly, there was a proposal <golang/go#16221> to add a T.Context() method to the testing package (for convenient cancellation on test completion), but that idea was subsequently abandoned in favor of sticking with ctx, cancel := context.WithCancel(context.Background()) in tests. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14128 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdsPIVrswtMgooKBpckS0-mie8lV16oks5rlgDbgaJpZM4Mb8XY> .

[nvanbenschoten]

I like the idea on the surface. That said, it would certainly require a lot more work than this initial change, primarily because we would need to factor all contexts into a single variable instead of just calling context.Background wherever needed, which is what most tests do at the moment. From the sounds of @andreimatei's doc, there might be more we could gain from a general testing context as well.

Either way though, that suggestion calls context.Background(), so we'll need to resolve this discussion first.

[a-robinson]

I'm with @nvanbenschoten on using context.Background in tests. It's the standard recommended usage, whereas context.TODO is meant for places that you plan on fixing later by plumbing in a real context (hence the TODO).

[tamird]

On Mon, Mar 13, 2017 at 11:20 PM, Nathan VanBenschoten < ***@***.***> wrote: That said, it would certainly require a lot more work than this initial change, primarily because we would need to factor all contexts into a single variable instead of just calling context.Background wherever needed, which is what most tests do at the moment.

Not really, you could just keep calling `testutils.NewContext(t)` wherever you needed a context.

[andreimatei]

I don't care very much what tests use, it's not particularly important. I'd argue that a) there's no reason to change it now and introduce diffs (about the diffs I'd care more) and b) context.Background() is the worst possible context to use in most of our tests (the ones that instantiate a server or a cluster) - but then again it doesn't really matter and so "the worst possible" is not too bad. context.Background() is completely empty, and so guaranteed to mix with the server's background work. context.TODO() we could in theory monkey-patch somehow, or temporarily change in the std lib source to isolate test operations.
Note that other libraries that use context.Background() in their tests don't have the amount of background work to compete with that we do.

@tamird's suggestion about creating a utility function to create a test-specific context is good, and has been made before. But, I personally think that, at this point, we need a bit more carrot and less stick for introducing diffs and forcing people to use yet another crdb idiosyncrasy, particularly given that it doesn't really matter. So, for example, I'd be against a linter that would check for uses of this function.
@nvanbenschoten, if you want to join the rebellion and produce some carrots, I can fish out that AppDash-in-tests branch that I had and then you can adopt it and show people why they should use something new.

---

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

Comments from Reviewable

[tamird]

Wow, that's a high bar for making a change. It seems pretty clear that this is not a CRDB idiosyncracy but rather documented convention, and establishing a clear pattern is reason enough to make this change. The diff "cost" only increases with time. Somehow tying this to appdash work is clearly inappropriate in my opinion.

…

On Mar 13, 2017 23:54, "Andrei Matei" ***@***.***> wrote: I don't care very much what tests use, it's not particularly important. I'd argue that a) there's no reason to change it now and introduce diffs (about the diffs I'd care more) and b) context.Background() is the worst possible context to use in most of our tests (the ones that instantiate a server or a cluster) - but then again it doesn't really matter and so "the worst possible" is not too bad. context.Background() is completely empty, and so guaranteed to mix with the server's background work. context.TODO() we could in theory monkey-patch somehow, or temporarily change in the std lib source to isolate test operations. Note that other libraries that use context.Background() in their tests don't have the amount of background work to compete with that we do. @tamird <https://github.com/tamird>'s suggestion about creating a utility function to create a test-specific context is good, and has been made before. But, I personally think that, at this point, we need a bit more carrot and less stick for introducing diffs and forcing people to use yet another crdb idiosyncrasy, particularly given that it doesn't really matter. So, for example, I'd be against a linter that would check for uses of this function. @nvanbenschoten <https://github.com/nvanbenschoten>, if you want to join the rebellion and produce some carrots, I can fish out that AppDash-in-tests branch that I had and then you can adopt it and show people why they should use something new. ------------------------------ Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed. ------------------------------ *Comments from Reviewable <https://reviewable.io:443/reviews/cockroachdb/cockroach/14128#-:-Kf9urCoM5ubkqysJkxf:b-6vrvql>* — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#14128 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABdsPHjSCdHhN8qylGrpTWBFP3Zi2yD9ks5rlg-MgaJpZM4Mb8XY> .

[nvanbenschoten]

Your mention of context.Background being the "the worst possible" Context to use in tests because it will "mix with the server's background work" and instead monkey-patching context.TODO leads me to believe there's a disconnect here. What exactly do you think could go wrong? They're both empty, immutable contexts without values or deadlines and can not be cancelled. They only serve as the root of Context trees, and because Contexts are never compared by address, they're not even distinguishable. For all intents and purposes, they are the same at runtime.

That said, I'd argue convention and establishing (well, following really!) a pattern is absolutely reason enough to make this change. Right now it's not clear which Context to use when writing tests, and that entropy is only going to grow if we don't formalize something. I'd much rather have one way to do something than two if the fix is this trivial.

In addition, calling it context.TODO was not a coincidence. They are supposed to be TODOs. So if adopting this pattern at the expense of a few diffs means we can remove 300+ TODOs from the codebase (roughly 20%), I'd argue that's a completely justifiable tradeoff.

[petermattis]

FWIW, I think using context.Background() in tests is fine. There is no urgency to using something "better" (i.e. testutils.Context(t)). And there is no need to leave these as context.TODO() as finding the usage of context.Background() in *_test.go files is easy enough.

[andreimatei]

I understand what TODO and Background are. They are indeed indistinguishable. Except if we were to change what TODO returns. If we switch everything to background (tests and server background work), then we lose this option. Anyway, monkey-patching was not real suggestion, just a hint about how TODO is theoretically slightly better.

I think the TODO should be read as "TODO: put something here that's better than Background()". So if you read it like that, removing the TODO and putting a Background() would be worse.

Here's my preferences, in order. Note that I don't care too much.

1. We don't change any code. If you want to give people guidance, we say that context.TODO() is to be used in tests that start a server, context.Background() in ones that don't. And we don't complain about it in reviews.
2. we do Tamir's testutils.Context(t), and we assign it to a ctx var at the beginning of the test and just use the var throughout.
3. we do testutils.Context(<no arg>), assign it to global variables called testCtx defined in test_main.go in various packages and use the globals.
4. we do testutils.Context(t) and call it directly everywhere where TODO and Background are called now.
5. we switch to Background. This can move to the top of the preferences if you feel strongly and it makes you happy. I care about your happiness :)

---

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

Comments from Reviewable

[RaduBerinde]

I used TODO in tests because I thought there might be a good reason to some day have a per-test context (like the proposed testutils.Context(t)) for capturing traces. The appdash work mentioned is one potential reason for this. We may also want to run various tests with Lighstep if it helps debugging. I think being able to look at a lightstep trace when starting to debug a test failure could save a lot of time.

I also think that it's a good convention to use Background() only when it is absolutely the correct thing to use (which should be very rare). It should not be used just because we don't have a better context to use; TODO should be used in that case. I think tests fall under this category.

That said, if we ever need to switch, it won't matter a whole lot if we have TODO or Background to fix up, so I don't feel strongly either way w.r.t changing them to Background now.

---

Comments from Reviewable

[nvanbenschoten]

There seems to be too much disagreement on this, and there's no point in unifying our approach if we're not going to maintain the convention. I've pulled out the first commit and am just going to move forward with the second two cleanups.

I'd only ask that if we're pushing to use context.TODO in tests because we don't have a better alternative, then we make a plan to eventually replace them with a better alternative. I've opened #14220 to track this.

---

Review status: 3 of 7 files reviewed at latest revision, 1 unresolved discussion.

---

pkg/sql/parser/eval.go, line 2487 at r2 (raw file):

Previously, tamird (Tamir Duberstein) wrote…

what is with all this error logging? seems like it should just be removed.

Done.

---

Comments from Reviewable

[tamird]

Interestingly, there was a proposal to add a T.Context() method to the testing package (for convenient cancellation on test completion), but that idea was subsequently abandoned in favor of sticking with ctx, cancel := context.WithCancel(context.Background()) in tests.

This was withdrawn because of the cleanup problem where the tests defer WaitForShutdown() but shutdown only happens when the context is cancelled, which only happens when the test exits.

Just sayin'.

---

Reviewed 66 of 66 files at r4, 1 of 1 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

---

Comments from Reviewable