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

Adding support for detecting nested contexts in function literals #18

Merged

Conversation

venkycode
Copy link
Contributor

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.

Copy link

@ccoVeille ccoVeille left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add tests, linter is updated, tests should be added to cover the expected case

@Crocmagnon
Copy link
Owner

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
Copy link
Contributor Author

venkycode commented Aug 25, 2024 via email

@venkycode venkycode force-pushed the feat/context-in-function-literal branch from e482fa4 to 89a1841 Compare August 25, 2024 19:59
@venkycode
Copy link
Contributor Author

venkycode commented Aug 25, 2024

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
Copy link
Contributor Author

venkycode commented Aug 25, 2024

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 Crocmagnon merged commit be0aa70 into Crocmagnon:master Aug 26, 2024
10 checks passed
@Crocmagnon
Copy link
Owner

Thanks! Released in v0.5.0 :shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants