Skip to content

[WIP] Create function like TestOnBorrow that includes context #442

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.

Sign up for GitHub

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

Jump to bottom
Open
MorrisLaw wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[WIP] Create function like TestOnBorrow that includes context #442

MorrisLaw wants to merge 1 commit into from

Conversation

@MorrisLaw
Copy link

@MorrisLaw MorrisLaw commented Nov 1, 2019

This is an initial PR to address issue #439

stevenh
stevenh requested changes Nov 1, 2019
View reviewed changes
Copy link
Collaborator

@stevenh stevenh left a comment

Choose a reason for hiding this comment

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

I'm not sure this can ever be of use if I'm honest as this is setup on the pool so the ctx with a timeout, which would be the desired use, will expire and resulting in all future calls failing.

redis/pool.go

// TestOnBorrowWithContext is the same as TestOnBorrow, but includes
// the context.
TestOnBorrowWithContext func(c Conn, t time.Time, ctx context.Context) error
Copy link
Collaborator

@stevenh stevenh Nov 1, 2019

Choose a reason for hiding this comment

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

Context should always be the first parameter, I would also drop the With so its just TestOnBorrowContext as per Dial and DailContext

@MorrisLaw
Copy link
Author

MorrisLaw commented Nov 1, 2019

the ctx with a timeout, which would be the desired use, will expire and resulting in all future calls failing.

I agree @stevenh , seems kind of pointless then... Should we just close this PR out?

@stevenh
Copy link
Collaborator

stevenh commented Apr 10, 2020

Closing based on timeout and other feedback.

@stevenh stevenh closed this Apr 10, 2020
@stevenh stevenh mentioned this pull request Apr 10, 2020
Open
@dcormier
Copy link
Contributor

dcormier commented Apr 13, 2020

I'm not sure this can ever be of use if I'm honest as this is setup on the pool so the ctx with a timeout, which would be the desired use, will expire and resulting in all future calls failing.

But wouldn't it be called with the context.Context passed to GetContext? I haven't dug into the code, but I would assume that's when TestOnBorrow would be called, so the context used should be correct. And if the context is canceled or timed out, then it should still be correct.

@stevenh
Copy link
Collaborator

stevenh commented Apr 13, 2020 •
edited
Loading

I take it back, @dcormier is correct, reopening.

@stevenh stevenh reopened this Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stevenh stevenh stevenh requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@MorrisLaw @stevenh @dcormier