search

0xERR0R / blocky

Fast and lightweight DNS proxy as ad-blocker for local network with many features
https://0xERR0R.github.io/blocky/
Apache License 2.0
4.38k stars 199 forks source link

Consistent context usage #1264

Open ThinkChaos opened 7 months ago

ThinkChaos commented 7 months ago

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

kwitsch commented 7 months ago

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 commented 7 months ago

.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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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

kwitsch commented 7 months ago

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 commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

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.