Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fixed a bug where a bad connection gets reused #5

Merged
merged 2 commits into from

2 participants

@jgrahamc

While using gomemcache under heavy load I saw a situation where odd errors would occur. Tracked it down to a bad := which should have been =. The result was that an error could get ignored and a bad connection could get reused.

jgrahamc added some commits
@jgrahamc jgrahamc Fix a problem where an error in onItem could result in a bad connecti…
…on being reused

If an error occurs inside the fn() that onItem calls it would have not been spotted by
condRelease() and that could result in a bad connection being placed back in the
pool of free connections. Under load the package was experiencing this problem manifested
in the following fashion:

  1. A connection gets an i/o timeout during a call to onItem()

  2. The error returned by the function called by onItem() is placed in a new
     err value (because of the :=) but the condRelease() has been set up with
     &err from a previous declaration of err. Therefore err is nil.

  3. condRelease() places the connection back in the free list.

  4. The connection is reused at some point and there's data waiting to be read
     from the previous memcache command that got the i/o timeout. This results
     in (typically) an error from the Sscanf line at line 405 because that line
     ends up trying to parse part of the response that was not previously read
     because of the i/o timeout.

This fix prevents this from happening. If an error occurs the connection will not be
reused.
1c05224
@jgrahamc jgrahamc Close connections that are being discarded
If a TCP connection to memcache is not being returned to the free list (because an error
has occurred on the connection and its reuse is not allowed), explicitly close the
connection.
b198fc8
@bradfitz bradfitz merged commit 66c15b0 into bradfitz:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 19, 2012
  1. @jgrahamc

    Fix a problem where an error in onItem could result in a bad connecti…

    jgrahamc authored
    …on being reused
    
    If an error occurs inside the fn() that onItem calls it would have not been spotted by
    condRelease() and that could result in a bad connection being placed back in the
    pool of free connections. Under load the package was experiencing this problem manifested
    in the following fashion:
    
      1. A connection gets an i/o timeout during a call to onItem()
    
      2. The error returned by the function called by onItem() is placed in a new
         err value (because of the :=) but the condRelease() has been set up with
         &err from a previous declaration of err. Therefore err is nil.
    
      3. condRelease() places the connection back in the free list.
    
      4. The connection is reused at some point and there's data waiting to be read
         from the previous memcache command that got the i/o timeout. This results
         in (typically) an error from the Sscanf line at line 405 because that line
         ends up trying to parse part of the response that was not previously read
         because of the i/o timeout.
    
    This fix prevents this from happening. If an error occurs the connection will not be
    reused.
  2. @jgrahamc

    Close connections that are being discarded

    jgrahamc authored
    If a TCP connection to memcache is not being returned to the free list (because an error
    has occurred on the connection and its reuse is not allowed), explicitly close the
    connection.
This page is out of date. Refresh to see the latest.
Showing with 3 additions and 1 deletion.
  1. +3 −1 memcache/memcache.go
View
4 memcache/memcache.go
@@ -183,6 +183,8 @@ func (cn *conn) extendDeadline() {
func (cn *conn) condRelease(err *error) {
if *err == nil || resumableError(*err) {
cn.release()
+ } else {
+ cn.nc.Close()
}
}
@@ -289,7 +291,7 @@ func (c *Client) onItem(item *Item, fn func(*Client, *bufio.ReadWriter, *Item) e
return err
}
defer cn.condRelease(&err)
- if err := fn(c, cn.rw, item); err != nil {
+ if err = fn(c, cn.rw, item); err != nil {
return err
}
return nil
Something went wrong with that request. Please try again.