Crocmagnon / fatcontext

detects nested contexts in loops or function literals
MIT License
8 stars 2 forks source link

Adding support for detecting nested contexts in function literals #18

Closed venkycode closed 2 weeks ago

venkycode commented 2 weeks ago

Function literals are usually called multiple times during their lifetime (consider middlewares, mapping functions etc). If they are modifying a context variable that was defined out of their scope - maybe in the parent function- it can very quickly bloat the original context variable. This can cause memory leak. I have experienced the same while working with golan with at my day job.

I think this change will help catch many such bugs.

I have also added testcases for this and have improved the identifier scope checking logic.

Crocmagnon commented 2 weeks ago

Hi! Thank you for your contribution :) I've updated master to always run all test suites + dropped support for go 1.21. Could you please rebase? At first it looks like your changes aren't compatible with go >=1.22 but I'd like to confirm that.

venkycode commented 2 weeks ago

Hi, i am currently away from my laptop. i'll update this soon.

On Sun, 25 Aug, 2024, 19:32 Gabriel Augendre, @.***> wrote:

Hi! Thank you for your contribution :) I've updated master to always run all test suites + dropped support for go 1.21. Could you please rebase? At first it looks like your changes aren't compatible with go >=1.22 but I'd like to confirm that.

— Reply to this email directly, view it on GitHub https://github.com/Crocmagnon/fatcontext/pull/18#issuecomment-2308861909, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJOI74G7SKFE46334YSEP2LZTHPYXAVCNFSM6AAAAABNB4S7R2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMBYHA3DCOJQHE . You are receiving this because you authored the thread.Message ID: @.***>

venkycode commented 2 weeks ago

After rebasing, i found tests are failing only for go 1.22 (and not for older version). The reason being go 1.22 changed the way Scope of an a function parameter variable is computed. For example,

func addOne(x int) int {
    return x + 1
}

In go 1.21, the scope of variable x started at { and ended at } In go 1.22, the scope of variable now starts at f and ends at }

I have made some changes to handle this.

venkycode commented 2 weeks ago

my new change also handles this case, which is giving a false positive on master

// this is fine because the context is created in the loop
    for {
        if ctx := context.Background(); doSomething() != nil {
            ctx = wrapContext(ctx)
        }
    }
Crocmagnon commented 2 weeks ago

Thanks! Released in v0.5.0 :shipit: