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

Fix vet warnings. #5816

Closed
wants to merge 1 commit into from
Closed

Fix vet warnings. #5816

wants to merge 1 commit into from

Conversation

ncabatoff
Copy link
Collaborator

After this change make vet no longer generates output.

@@ -689,7 +689,9 @@ START:
}

if timeout != 0 {
ctx, _ = context.WithTimeout(ctx, timeout)
var cancel context.CancelFunc
ctx, cancel = context.WithTimeout(ctx, timeout)
Copy link
Member

Choose a reason for hiding this comment

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

IIRC, this was done intentionally. I wish there was a comment explaining why but maybe we should test this out before we merge it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for pointing that out. You're right: back in #4987 you removed an existing call to the cancelfunc that was already there. Some background is given in #4973 and #4979, where the idea of doing it in a defer was discussed. It looks like that approach was rejected because of @jefferai 's comment that

The problem with deferring is that we support retries so these could pile up

but that confuses me because there's at best a single retry in this scope (namely, when redirectCount==0).

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