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

propogate err to condRelease in withAddrRw #170

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

Conversation

jlisthood
Copy link

@jlisthood jlisthood commented Sep 12, 2023

Currently, withAddrRw always calls defer cn.condRelease(&err) with an err that is nil. This means no matter what error happens in fn, the connection will get sent back to the pool even if there is a non-resumable error. This isn't ideal because we could be sending bad connections back to the pool.

This PR matches the implementation with onItem that seems to handle the error correctly.

@jlisthood
Copy link
Author

@bradfitz apologies for the ping, but do you believe change is necessary? I imagine the worst case is that withAddrRw may put bad connections back into the connection pool on non-recoverable errors.

In our private fork of gomemcache, we actually changed onItem to match withAddrRw's error handling since a linter recommended just returning the result of the passed in function instead of capturing it in the err variable first.

We did this but then we found that during network difficulties we would eventually see get requests failing with unexpected responses memcache: unexpected line in get response: "STORED\r\n", which indicates that connections from failed set calls were actually getting used and incorrectly read later for get requests. This happened infrequently for months and we reverted that linter suggestion and the errors went away completely.

It seems like withAddrRw would have the same exact issue, but we have never seen evidence of this happening in our logs. We patched this change in our fork just in case, but would love your opinion on it as well.

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.

1 participant