Skip to content

Conversation

@Rahul-Lashkari
Copy link
Contributor

@Rahul-Lashkari Rahul-Lashkari commented Dec 6, 2025

  • replaced the deprecated golang.org/x/net/context with the standard library context package
  • removed defaultContext() and fixed context leaks
  • make format, tidy, test checked

@dims
Copy link
Collaborator

dims commented Dec 8, 2025

ci job fail :(

Run source build/config/plain.sh
>> upgrading golangci-lint from  to 2.6.2
golangci/golangci-lint info checking GitHub for tag 'v2.6.2'
golangci/golangci-lint info found version: 2.6.2 for v2.6.2/linux/amd64
golangci/golangci-lint info installed /home/runner/go/bin/golangci-lint
>> running golangci-lint using configuration at .golangci.yml
Error: container/docker/docker.go:37:7: lostcancel: the cancel function returned by context.WithTimeout should be called, not discarded, to avoid a context leak (govet)
	ctx, _ := context.WithTimeout(context.Background(), dockerTimeout)
	     ^
Error: container/docker/plugin.go:60:8: lostcancel: the cancel function returned by context.WithTimeout should be called, not discarded, to avoid a context leak (govet)
		ctx, _ := context.WithTimeout(context.Background(), startupTimeout)
		     ^
2 issues:
* govet: 2
make: *** [Makefile:89: lint] Error 1
Error: Process completed with exit code 2.

@thaJeztah
Copy link
Contributor

ci job fail :(

Looks like it's an existing issue that wasn't detected by the linter due to the legacy context.

But the linter definitely is correct, and the existing implementation doesn't look good. Perhaps the defaultContext utility should either be removed, or otherwise updated to return the cancel, so that callers can handle the defer cancel(). Given that it's a very shallow wrapper, perhaps removing the utility, and inlining the code where it's used would be more transparent;

func defaultContext() context.Context {
ctx, _ := context.WithTimeout(context.Background(), dockerTimeout)
return ctx
}

@Rahul-Lashkari can you look at making those changes?

@Rahul-Lashkari
Copy link
Contributor Author

ci job fail :(

Looks like it's an existing issue that wasn't detected by the linter due to the legacy context.

But the linter definitely is correct, and the existing implementation doesn't look good. Perhaps the defaultContext utility should either be removed, or otherwise updated to return the cancel, so that callers can handle the defer cancel(). Given that it's a very shallow wrapper, perhaps removing the utility, and inlining the code where it's used would be more transparent;

func defaultContext() context.Context {
ctx, _ := context.WithTimeout(context.Background(), dockerTimeout)
return ctx
}

@Rahul-Lashkari can you look at making those changes?

yeah sure on it! thanks for guidance!

@Rahul-Lashkari
Copy link
Contributor Author

PTAL!

Copy link
Contributor

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

Thanks! left some minor comments; also if you could squash the commits, so that there's a single commit in the PR

I'm not a maintainer on this repository, so can't trigger CI to run 😅

Copy link
Contributor

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@dims dims enabled auto-merge December 9, 2025 16:16
@dims dims added this pull request to the merge queue Dec 9, 2025
Merged via the queue into google:master with commit 8304937 Dec 9, 2025
7 checks passed
@Rahul-Lashkari
Copy link
Contributor Author

thanks for merging!

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