Multiple connections return to stack by ResponseBody.close()

[GoogleCodeExporter]

`ResponseBody` on its `close()` method calls callback that returns response's 
connection to the stack. But in some cases (when `read()` is called after end 
of stream or manual `close()` call) it can return the same connection many 
times that cause errors in further request to storage.

Patch and test attached.

Original issue reported on code.google.com by daevaorn on 13 Mar 2011 at 7:14

Attachments:

• response-close-callback.diff

[GoogleCodeExporter]

What I need to do to get some feedback on this issue?

Original comment by daevaorn on 16 Apr 2011 at 7:33

[GoogleCodeExporter]

Could you provide example to reproduce this error?

Original comment by kxepal on 16 Apr 2011 at 7:41

[GoogleCodeExporter]

There is the example in the patch's test case.

Original comment by daevaorn on 16 Apr 2011 at 7:43

[GoogleCodeExporter]

yes, but it's a little out of real world: I've never thought about such usage 
of http.ResponseBody class(:

Original comment by kxepal on 16 Apr 2011 at 7:57

[GoogleCodeExporter]

Ok. I agree that the test just illustrate multiple callback call.

The real world example:

{{{
# ... creating `database` object
>>> resource = database.resource('_all_docs')
>>> code, headers, response = resource.get()
>>> response.read() # read and parse data
... a lot of data ...
>>> resource.session.conns
{('http', 'localhost:5984'): [<httplib.HTTPConnection instance at 0x25bf1b8]}
>>> response.read() # some json parser require to get empty string as mark the 
end of the input stream
''
>>> resource.session.conns # now session connections stack has to items 
reference to the same http connection
{('http', 'localhost:5984'): [<httplib.HTTPConnection instance at 0x25bf1b8>,
                              <httplib.HTTPConnection instance at 0x25bf1b8>]}
>>> resource.get() # make another call to same or different large resource
(200,
 <httplib.HTTPMessage instance at 0x2681758>,
 <couchdb.http.ResponseBody object at 0x26637d0>)
>>> resource.get() # and another one before reading entire response from the 
previous
---------------------------------------------------------------------------
ResponseNotReady                          Traceback (most recent call last)

<ipython console> in <module>()

/nfs/compat/python/site-packages/couchdb/http.pyc in get(self, path, headers, 
**params)
    421 
    422     def get(self, path=None, headers=None, **params):
--> 423         return self._request('GET', path, headers=headers, **params)
    424 
    425     def head(self, path=None, headers=None, **params):

/nfs/compat/python/site-packages/couchdb/http.pyc in _request(self, method, 
path, body, headers, **params)
    466         return self.session.request(method, url, body=body,
    467                                     headers=all_headers,
--> 468                                     credentials=self.credentials)
    469 
    470 

/nfs/compat/python/site-packages/couchdb/http.pyc in request(self, method, url, 
body, headers, credentials, num_redirects)
    285                     raise
    286 
--> 287         resp = _try_request_with_retries(iter(self.retry_delays))
    288         status = resp.status
    289 

/nfs/compat/python/site-packages/couchdb/http.pyc in 
_try_request_with_retries(retries)
    245             while True:
    246                 try:
--> 247                     return _try_request()
    248                 except socket.error, e:
    249                     ecode = e.args[0]

/nfs/compat/python/site-packages/couchdb/http.pyc in _try_request()
    274                             conn.send(('%x\r\n' % len(chunk)) + chunk + '\r\n')
    275                         conn.send('0\r\n\r\n')
--> 276                 return conn.getresponse()
    277             except BadStatusLine, e:
    278                 # httplib raises a BadStatusLine when it cannot read the status

/usr/lib/python2.6/httplib.pyc in getresponse(self)
    974         #

    975         if self.__state != _CS_REQ_SENT or self.__response:
--> 976             raise ResponseNotReady()
    977 
    978         if self.debuglevel > 0:

ResponseNotReady:   
}}}

The same example but whit highlighting -- http://pastebin.com/uwX9ncn0

Original comment by daevaorn on 16 Apr 2011 at 8:25

[GoogleCodeExporter]

Up!

Original comment by daevaorn on 23 May 2011 at 9:21

[GoogleCodeExporter]

By the way, is there any reasons to keep stack of same connections objects for 
single resource? I suppose this is incorrect, because only first one will be 
usable, others would generate same error. So patch doesn't fixes bug, but hides 
it more carefully. Alternative solution in attachments.

Original comment by kxepal on 23 May 2011 at 9:55

[GoogleCodeExporter]

damn, sorry, forgot press ctrl+s to save correct patch version(: list check is 
the solution, but there is set object for such tasks.

Original comment by kxepal on 23 May 2011 at 10:12

Attachments:

• issue-170.patch

[GoogleCodeExporter]

Committed daevaorn's patch in r026ad8201ffd. Thanks!

I don't think we should change the connection pool implementation to use a 
set() as it just hides any other problems. Better to fix the source of the 
error whenever possible.

Original comment by matt.goo...@gmail.com on 28 May 2011 at 8:27

• Changed state: Fixed

[GoogleCodeExporter]

I wouldn't agree, because source of problem is in connection stack, not in how 
the ResponseBody operates with it. If there would be added similar to 
ResponseBody object, should it care about same problem?

Original comment by kxepal on 28 May 2011 at 8:37