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

add DoContext and ReceiveContext,use context to control the life #537

Merged
merged 12 commits into from Sep 30, 2021
Merged

add DoContext and ReceiveContext,use context to control the life #537

merged 12 commits into from Sep 30, 2021

Conversation

chenjie199234
Copy link
Contributor

@chenjie199234 chenjie199234 commented Nov 20, 2020

context is mostly used when develop a project

I think the with timeout function can also change to this rule
don't override the readtime

can this be merged and make a new tag?my project need this
by the way,can we fix the tag version number?

@stevenh
Copy link
Collaborator

stevenh commented Nov 24, 2020

Why not just use pool.GetContext(ctx)?

@chenjie199234
Copy link
Contributor Author

chenjie199234 commented Nov 24, 2020

@stevenh pool.GetContext(ctx) only get a connection.The ctx will not effect the Do function.

pool := &redis.Pool{}
pool.DialCOntext = func(ctx context.Context){
    return redis.DialContext(ctx,"tcp","127.0.0.1:6379"/*here may be some options like DialReadTimeout*/)
}
func BusinessLogic(ctx context.Context) error{
    c,e:=pool.GetContext(ctx)
    //ctx didn't expired now,i can get the connection
    if e!=nil{
        return e
    }
    //.....here did some other logic
    //then the ctx is expired
    //now call the Do function
    c.Do("SET","test","abc")
    //the Do function will success.
    //because the Do function will use the readtimeout which was setted when the connection was created.but it is 0.
}

1.when a connection create,the connection has 2 timeout,readtimeout and writetimeout,they are setted by DialReadTimeout and DialWriteTimeout
2.when a Do was called,the readtimeout will be used if it is not zero.
3.so the context in pool.GetContext(ctx) will only effect the connection create,it will not effect the command exec.

The comment above the GetContext also said this.
If want to control the command's life,redigo support to use the DoWithTimeout function.But I Think DoContext is more useful then DoWithTimeout

@chenjie199234
Copy link
Contributor Author

chenjie199234 commented Nov 25, 2020

@stevenh can this be merged?The opensource's working efficiency is really a nightmare. 0_0!!!

Copy link
Collaborator

@stevenh stevenh left a comment

Few things to address not mentioned in the inline comments:

  • How does this deal cancelled context on connections? See #351 for some interesting points, it's not as easy as it first appears.
  • Needs tests

redis/conn.go Outdated Show resolved Hide resolved
redis/conn.go Outdated Show resolved Hide resolved
redis/conn.go Outdated Show resolved Hide resolved
redis/conn.go Show resolved Hide resolved
redis/pool.go Outdated Show resolved Hide resolved
redis/redis.go Show resolved Hide resolved
redis/redis.go Outdated Show resolved Hide resolved
@chenjie199234
Copy link
Contributor Author

chenjie199234 commented Nov 26, 2020

@stevenh
Cancel context supported.

Copy link
Collaborator

@stevenh stevenh left a comment

No need to force push as it makes it hard to see the changes I will squash the commits to one on merge anyway.

Here's some more feedback on top of the previous pieces.

redis/conn.go Outdated Show resolved Hide resolved
redis/conn.go Show resolved Hide resolved
redis/conn.go Show resolved Hide resolved
redis/pool.go Show resolved Hide resolved
redis/pool.go Show resolved Hide resolved
redis/redis.go Outdated Show resolved Hide resolved
redis/redis.go Outdated Show resolved Hide resolved
@NitroCao
Copy link

NitroCao commented Dec 15, 2020

Any progress on this? It's indeed important :(

redis/redis.go Outdated Show resolved Hide resolved
redis/redis.go Outdated Show resolved Hide resolved
@pjebs
Copy link

pjebs commented Dec 15, 2020

My thoughts

Why are we redefining a brand new context cancelation error type: var ErrContextCanceled = errors.New("redis: context canceled")

Can't we use the one from standard library: https://godoc.org/context#pkg-variables

Lots of people like to check the returned error and compare it to the standard library to distinguish between whether the context was cancelled or whether a deadline exceeded.

  1. This PR increases the API surface substantially (and in my opinion excessively).

All that is needed is this added to a v2 Conn interface:

func DoContext(ctx context.Context, commandName string, args ...interface{}) (reply interface{}, err error) {

There should be no need for adding timeout/deadline facilities to make it "appear" more convenient for developers.
Developers can easily do it themselves via the context package.

Alternatively, Conn2 interface can be defined which embeds Conn interface.

@pjebs
Copy link

pjebs commented Dec 15, 2020

I think maintainer wants to go in this direction: #351

@NitroCao
Copy link

NitroCao commented Dec 16, 2020

Ah, I think I should consider using github.com/go-redis/redis instead :)

Copy link
Collaborator

@stevenh stevenh left a comment

My thoughts

Why are we redefining a brand new context cancelation error type: var ErrContextCanceled = errors.New("redis: context canceled")

Can't we use the one from standard library: https://godoc.org/context#pkg-variables

Lots of people like to check the returned error and compare it to the standard library to distinguish between whether the context was cancelled or whether a deadline exceeded.

Good call, we should definitely do that.

  1. This PR increases the API surface substantially (and in my opinion excessively).

All that is needed is this added to a v2 Conn interface:

func DoContext(ctx context.Context, commandName string, args ...interface{}) (reply interface{}, err error) {

There should be no need for adding timeout/deadline facilities to make it "appear" more convenient for developers.
Developers can easily do it themselves via the context package.

Alternatively, Conn2 interface can be defined which embeds Conn interface.

Not sure I follow you there, could you clarify?

redis/redis_test.go Outdated Show resolved Hide resolved
redis/redis_test.go Outdated Show resolved Hide resolved
@stevenh
Copy link
Collaborator

stevenh commented Dec 18, 2020

Ah, I think I should consider using github.com/go-redis/redis instead :)

Is there something that is missing that is driving that comment?

@chenjie199234
Copy link
Contributor Author

chenjie199234 commented Dec 24, 2020

@stevenh @NitroCao @pjebs @ash2k
sorry guys,work and life have some change,i will find some time to fix the comments,but it may have some delay

chenjie199234 and others added 4 commits Dec 24, 2020
fix typo

Co-authored-by: Mikhail Mazurskiy <126021+ash2k@users.noreply.github.com>
fix typo

Co-authored-by: Mikhail Mazurskiy <126021+ash2k@users.noreply.github.com>
@chenjie199234
Copy link
Contributor Author

chenjie199234 commented Dec 28, 2020

@pjebs @stevenh already changed context error to std lib's error.
test file fixed too

@chenjie199234 chenjie199234 requested a review from stevenh Dec 29, 2020
redis/redis.go Outdated Show resolved Hide resolved
redis/redis.go Outdated Show resolved Hide resolved
redis/redis.go Outdated Show resolved Hide resolved
redis/redis.go Outdated Show resolved Hide resolved
redis/redis.go Outdated Show resolved Hide resolved
redis/redis_test.go Outdated Show resolved Hide resolved
@chenjie199234 chenjie199234 requested a review from stevenh Dec 30, 2020
redis/conn.go Outdated Show resolved Hide resolved
@TheFireMike
Copy link

TheFireMike commented May 17, 2021

Any updates on this? We would really like to use this library in our Thola project, but Context support is critical for us.

@stevenh stevenh merged commit 56d6448 into gomodule:master Sep 30, 2021
1 check passed
@stevenh
Copy link
Collaborator

stevenh commented Sep 30, 2021

@TheFireMike this is now merged, sorry I'm not been getting some github emails, so didn't realise this was now complete.

Thanks to everyone for their contributions to this PR.

@KebSeb
Copy link

KebSeb commented Dec 2, 2021

may i ask, why i cant find DoContext in the current realease (v1.8.5)?

@stevenh
Copy link
Collaborator

stevenh commented Dec 2, 2021

It was merged after v1.8.5.

I've just created v1.8.6

@KebSeb
Copy link

KebSeb commented Dec 2, 2021

this is awesome, thank you very much

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.

None yet

8 participants