Consistent context usage

[ThinkChaos]

Let's make a checklist (please edit this post if you have more ideas)!

• .golang-ci.yml: enable context related linters Refactoring: FileDownloader #1281
• cmd/: have a local change for this, will make MR after v0.24
• ListCache.Refresh
• log/
  I want to support passing a logger via a context, so we can get rid of Request.Log and just have the right logger be chosen by r.log(ctx). Started work on that, but depends on other stuff first. refactor(log): store log in context so it's automatically propagated #1356
• lists: Refactoring: FileDownloader #1281
  • FileDownloader.DownloadFile Refactoring: FileDownloader #1281
• redis/ Refactoring Redis #1271
• resolver.Resolver refactor: make use of contexts in more places #1261
• server.go Shutdown -> ShutdownContext Refactoring server.go #1277
• server.go remove context.Background() in favor of correct Context Refactoring server.go #1277
• server.go cancel go routines on ctx.Done() not necessary since the servers inside are closed with a context

[kwitsch]

I want to support passing a logger via a context, so we can get rid of Request.Log and just have the right logger be chosen by r.log(ctx). Started work on that, but depends on other stuff first.

That sounds interesting. ♥️

[kwitsch]

.golang-ci.yml: enable context related linters

Did this on a dev branch and fell in a rabbit hole of removing context.Background from the unit tests 😵‍💫

I'm fairly certain that we fix a lot flakiness from the tests if we actually use the ginkgo contexts...

[ThinkChaos]

I'm fairly certain that we fix a lot flakiness from the tests if we actually use the ginkgo contexts...

Hmm I don't see how it would but could be wrong. What are you thinking?

[kwitsch]

Hmm I don't see how it would but could be wrong. What are you thinking?

Sorry had to be more specific.
I was talking about the e2e-tests which are currently flaky ridden. 😅
The false error runs in most cases looked like a timing problem in the resource allocation and freeing to me.

My theory: since the background context isn't finished after the spec execution it doesn't close services that aren't expected to be in the following spec. But sometimes they are there and produce errors by blocking things.

Background: I tried to remove the http.Get with a context aware one and passing the ginkgo context to it.
That made the tests even more flaky...
After I slowly replaced the background context with the ginkgo one it got better.

Do I miss something and it's only a coincidence? 🤔

[ThinkChaos]

I'm definitely not that familiar with the e2e tests, but sounds quite plausible!

[kwitsch]

Everything(except the tests) should use the correct context after the last merge. 👍

My assessment was too early.
I'll add more checkpoints after I find them.

No context.Background() found in actual code(besides tests) after the config refactoring. 👍

[0xERR0R]

@ThinkChaos @kwitsch Hey, I'd like to make release in the next 1-2 weeks. This is last issue scheduled for 0.23. Is there something in progress and will be implemented or should I change fix version to 0.24?

[kwitsch]

@ThinkChaos @kwitsch Hey, I'd like to make release in the next 1-2 weeks. This is last issue scheduled for 0.23. Is there something in progress and will be implemented or should I change fix version to 0.24?

@0xERR0R Sorry I forgot to update my last comment. 😅
I'm currently not working on this issue and the only open task is the log refactoring that @ThinkChaos intended to do.
Maybe we shoud open a seperate issue for it?🤔

[ThinkChaos]

I'll do a search for context. and update the list/close this issue depending on what I find.
Either way, anything here isn't a blocker for the release IMO since it's internal refactoring.
EDIT: added cmd/ and ListCache.Refresh to the TODO.

There's one thing I noticed I broke with #1261 and I think we should fix before v0.24. See #1266.