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

Avoid QUIT for broken connections #2336

Merged
merged 7 commits into from
Feb 17, 2021

Conversation

sazzad16
Copy link
Contributor

@sazzad16 sazzad16 commented Jan 4, 2021

Note: Found it hard to write proper test codes. We may need to mock for this purpose.

@sazzad16 sazzad16 added this to the 3.5.0 milestone Jan 4, 2021
@sazzad16 sazzad16 requested a review from gkorland January 4, 2021 07:53
@yangbodong22011
Copy link
Collaborator

Hi, @sazzad16, the code look good to me, but i don't think it will solve #2108 #2105

Under current jedis netWork protocol, if a operation timeouted, will set borken = true, then the socket will be closed. The reason is the socket may be return data after sometime, this may cause next operation get error response.

About this issue, i think the only way to avoid close connection is increase soTimeout.

@sazzad16
Copy link
Contributor Author

@yangbodong22011 The object will be returned to the pool as a broken resource. The pool will remove it and never use it again. If necessarily, the pool will create a new resource which will have a new socket. Do you think it will still be a problem?

@yangbodong22011
Copy link
Collaborator

@yangbodong22011 The object will be returned to the pool as a broken resource. The pool will remove it and never use it again. If necessarily, the pool will create a new resource which will have a new socket. Do you think it will still be a problem?

This is the current logic, and it is correct. but create a new resource will cause #2105 (comment) .
One way to avoid connection broken is to avoid timeout, that is, increase soTimeout.

@sazzad16
Copy link
Contributor Author

but create a new resource will cause #2105 (comment) .
One way to avoid connection broken is to avoid timeout, that is, increase soTimeout.

The goal of this PR is not to avoid creating new connections and solve #2105. The goal here is to avoid executing a Redis command (QUIT) with a connection which is already broken while trying to execute some other command.

If you are not convinced that this will resolve #2108, we can still keep the issue open and move forward with this PR.

@sazzad16 sazzad16 modified the milestones: 3.5.0, 3.6.0 Jan 19, 2021
@sazzad16 sazzad16 force-pushed the avoid-quit-while-broken branch from bb8ecc5 to ab9ed11 Compare February 7, 2021 11:16
@sazzad16 sazzad16 merged commit 6f08468 into redis:master Feb 17, 2021
@sazzad16 sazzad16 deleted the avoid-quit-while-broken branch February 17, 2021 14:59
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