Commit bac6ea51 authored by Michael Hanselmann's avatar Michael Hanselmann
Browse files

http.client: Remove use of PycURL's “reset” function



We don't re-use cURL objects anymore, so there's no need to reset them.
PycURL 7.19.0 has a reference counting bug leading to a crash after
a certain number of performed requests.

Since a unittest depended on this function for a test, it is replaced
with a per-request completion callback, which could also be used by
users of the HTTP client.

Unittests are updated and a test for the request object's “nice name”,
used by the lock monitor, is included.
Signed-off-by: default avatarMichael Hanselmann <hansmi@google.com>
Reviewed-by: default avatarGuido Trotter <ultrotter@google.com>
parent 4fd2660d
......@@ -35,7 +35,8 @@ from ganeti import locking
class HttpClientRequest(object):
def __init__(self, host, port, method, path, headers=None, post_data=None,
read_timeout=None, curl_config_fn=None, nicename=None):
read_timeout=None, curl_config_fn=None, nicename=None,
completion_cb=None):
"""Describes an HTTP request.
@type host: string
......@@ -58,10 +59,14 @@ class HttpClientRequest(object):
@type nicename: string
@param nicename: Name, presentable to a user, to describe this request (no
whitespace)
@type completion_cb: callable accepting this request object as a single
parameter
@param completion_cb: Callback for request completion
"""
assert path.startswith("/"), "Path must start with slash (/)"
assert curl_config_fn is None or callable(curl_config_fn)
assert completion_cb is None or callable(completion_cb)
# Request attributes
self.host = host
......@@ -71,6 +76,7 @@ class HttpClientRequest(object):
self.read_timeout = read_timeout
self.curl_config_fn = curl_config_fn
self.nicename = nicename
self.completion_cb = completion_cb
if post_data is None:
self.post_data = ""
......@@ -220,14 +226,11 @@ class _PendingRequest:
req.resp_body = self._resp_buffer_read()
# Ensure no potentially large variables are referenced
try:
# Only available in PycURL 7.19.0 and above
reset_fn = curl.reset
except AttributeError:
curl.setopt(pycurl.POSTFIELDS, "")
curl.setopt(pycurl.WRITEFUNCTION, lambda _: None)
else:
reset_fn()
curl.setopt(pycurl.POSTFIELDS, "")
curl.setopt(pycurl.WRITEFUNCTION, lambda _: None)
if req.completion_cb:
req.completion_cb(req)
class _NoOpRequestMonitor: # pylint: disable=W0232
......
......@@ -377,6 +377,22 @@ class TestClientRequest(unittest.TestCase):
cr = http.client.HttpClientRequest("localhost", 1234, "GET", "/version")
self.assertEqual(cr.post_data, "")
def testCompletionCallback(self):
for argname in ["completion_cb", "curl_config_fn"]:
kwargs = {
argname: NotImplementedError,
}
cr = http.client.HttpClientRequest("localhost", 14038, "GET", "/version",
**kwargs)
self.assertEqual(getattr(cr, argname), NotImplementedError)
for fn in [NotImplemented, {}, 1]:
kwargs = {
argname: fn,
}
self.assertRaises(AssertionError, http.client.HttpClientRequest,
"localhost", 23150, "GET", "/version", **kwargs)
class _FakeCurl:
def __init__(self):
......@@ -619,14 +635,24 @@ class TestProcessRequests(unittest.TestCase):
def cfg_fn(port, curl):
curl.opts["__port__"] = port
def _LockCheckReset(monitor, curl):
def _LockCheckReset(monitor, req):
self.assertTrue(monitor._lock.is_owned(shared=0),
msg="Lock must be owned in exclusive mode")
curl.opts["__lockcheck__"] = True
assert not hasattr(req, "lockcheck__")
setattr(req, "lockcheck__", True)
def _BuildNiceName(port, default=None):
if port % 5 == 0:
return "nicename%s" % port
else:
# Use standard name
return default
requests = \
[http.client.HttpClientRequest("localhost", i, "POST", "/version%s" % i,
curl_config_fn=compat.partial(cfg_fn, i))
curl_config_fn=compat.partial(cfg_fn, i),
completion_cb=NotImplementedError,
nicename=_BuildNiceName(i))
for i in range(15176, 15501)]
requests_count = len(requests)
......@@ -641,14 +667,27 @@ class TestProcessRequests(unittest.TestCase):
self.assertTrue(compat.all(isinstance(curl, _FakeCurl)
for curl in handles))
# Prepare for lock check
for req in requests:
assert req.completion_cb is NotImplementedError
if use_monitor:
req.completion_cb = \
compat.partial(_LockCheckReset, lock_monitor_cb.GetMonitor())
for idx, curl in enumerate(handles):
port = curl.opts["__port__"]
try:
port = curl.opts["__port__"]
except KeyError:
self.fail("Per-request config function was not called")
if use_monitor:
# Check if lock information is correct
lock_info = lock_monitor_cb.GetMonitor().GetLockInfo(None)
expected = \
[("rpc/localhost/version%s" % handle.opts["__port__"], None,
[("rpc/%s" % (_BuildNiceName(handle.opts["__port__"],
default=("localhost/version%s" %
handle.opts["__port__"]))),
None,
[threading.currentThread().getName()], None)
for handle in handles[idx:]]
self.assertEqual(sorted(lock_info), sorted(expected))
......@@ -664,21 +703,17 @@ class TestProcessRequests(unittest.TestCase):
pycurl.RESPONSE_CODE: response_code,
}
# Unset options which will be reset
assert not hasattr(curl, "reset")
if use_monitor:
setattr(curl, "reset",
compat.partial(_LockCheckReset, lock_monitor_cb.GetMonitor(),
curl))
else:
self.assertFalse(curl.opts.pop(pycurl.POSTFIELDS))
self.assertTrue(callable(curl.opts.pop(pycurl.WRITEFUNCTION)))
# Prepare for reset
self.assertFalse(curl.opts.pop(pycurl.POSTFIELDS))
self.assertTrue(callable(curl.opts.pop(pycurl.WRITEFUNCTION)))
yield (curl, msg)
if use_monitor:
self.assertTrue(compat.all(curl.opts["__lockcheck__"]
for curl in handles))
self.assertTrue(compat.all(req.lockcheck__ for req in requests))
if use_monitor:
self.assertEqual(lock_monitor_cb.GetMonitor(), None)
http.client.ProcessRequests(requests, lock_monitor_cb=lock_monitor_cb,
_curl=_FakeCurl,
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment