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.

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

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

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

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

wants to merge 1 commit into from

Conversation

MorrisLaw
Copy link

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
@@ -141,6 +141,10 @@ type Pool struct {
// closed.
TestOnBorrow func(c Conn, t time.Time) error

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

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

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

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

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.

None yet
3 participants
@MorrisLaw @stevenh @dcormier