Repository: Azure/WALinuxAgent
Pull request #1154 (2018-05-08 16:11:08)

set the Connection close header
There is a slow-ish memory leak in the agent's HTTP request library.  The leak has been confirmed on OpenSuSE and Debian thus far. I have not been able to reproduce it on Ubuntu, RHEL, or CentOS.

I created a [branch](https://github.com/Azure/WALinuxAgent/tree/pr-tracemalloc) with tracemalloc instrumentation to help track down the issue. The leak appeared with the following file and line context.

```text
Top 50 lines
#1: python3.5/socket.py:647: 13609.4 KiB
    self._sock = None
```

This was just useful enough to identify that the issue had to do with sockets.  (tracemalloc supports grabbing a larger stacktrace around the memory allocation source, but I was not able to get that additional context.  I have not debugged why.)  The couple of bare socket calls that the agent makes are infrequent. The agent does make a lot of HTTP requests, so I started with the HTTP library.

The first thing I noticed is that the agent never explicitly cleans up an HTTP connection when it is done with it.  Note that the following method never calls `resp.connection.close()`.

```python
    def put_block_blob(self, url, data):
        logger.verbose("Put block blob")
        headers = self.get_block_blob_headers(len(data))
        resp = self.client.call_storage_service(restutil.http_put, url, data, headers)
        if resp.status != httpclient.CREATED:
            raise UploadError(
                "Failed to upload block blob: {0}".format(resp.status))
```

I thought this was fine. I would expect Python to clean up once all references were removed, but this does not appear to be true at least in the case of Debian/OpenSuSE.  I could fix the code to explicitly close the connection, but there are many places and branches where the agent makes HTTP requests.  The HTTP library returns the HTTPResponse and not the connection, which is generally fine because you can **usually** reach the connection via the response. Another fix, that appears to be working, is to explicitly set the HTTP Connection header to close.

In my travels, I found that the Python requests library also had this issue.  Their fix was to explicitly close the connection after use.  I think that we should make that fix too, but that is a bigger and more invasive change.  I would prefer to ship this fix as a hotfix, and leave the bigger fix for another release.


Patch:
diff --git a/azurelinuxagent/common/utils/restutil.py b/azurelinuxagent/common/utils/restutil.py
index fc9aac93..5ceb4c94 100644
--- a/azurelinuxagent/common/utils/restutil.py
+++ b/azurelinuxagent/common/utils/restutil.py
@@ -170,6 +170,8 @@ def _http_request(method, host, rel_uri, port=None, data=None, secure=False,
                   headers=None, proxy_host=None, proxy_port=None):
 
     headers = {} if headers is None else headers
+    headers['Connection'] = 'close'
+
     use_proxy = proxy_host is not None and proxy_port is not None
 
     if port is None:


Test patch:
diff --git a/tests/utils/test_rest_util.py b/tests/utils/test_rest_util.py
index a864884a..adeb8141 100644
--- a/tests/utils/test_rest_util.py
+++ b/tests/utils/test_rest_util.py
@@ -195,7 +195,7 @@ class TestHttpOperations(AgentTestCase):
         ])
         HTTPSConnection.assert_not_called()
         mock_conn.request.assert_has_calls([
-            call(method="GET", url="/bar", body=None, headers={'User-Agent': HTTP_USER_AGENT})
+            call(method="GET", url="/bar", body=None, headers={'User-Agent': HTTP_USER_AGENT, 'Connection': 'close'})
         ])
         self.assertEqual(1, mock_conn.getresponse.call_count)
         self.assertNotEquals(None, resp)
@@ -218,7 +218,7 @@ class TestHttpOperations(AgentTestCase):
             call("foo", 443, timeout=10)
         ])
         mock_conn.request.assert_has_calls([
-            call(method="GET", url="/bar", body=None, headers={'User-Agent': HTTP_USER_AGENT})
+            call(method="GET", url="/bar", body=None, headers={'User-Agent': HTTP_USER_AGENT, 'Connection': 'close'})
         ])
         self.assertEqual(1, mock_conn.getresponse.call_count)
         self.assertNotEquals(None, resp)
@@ -242,7 +242,7 @@ class TestHttpOperations(AgentTestCase):
         ])
         HTTPSConnection.assert_not_called()
         mock_conn.request.assert_has_calls([
-            call(method="GET", url="http://foo:80/bar", body=None, headers={'User-Agent': HTTP_USER_AGENT})
+            call(method="GET", url="http://foo:80/bar", body=None, headers={'User-Agent': HTTP_USER_AGENT, 'Connection': 'close'})
         ])
         self.assertEqual(1, mock_conn.getresponse.call_count)
         self.assertNotEquals(None, resp)
@@ -267,7 +267,7 @@ class TestHttpOperations(AgentTestCase):
             call("foo.bar", 23333, timeout=10)
         ])
         mock_conn.request.assert_has_calls([
-            call(method="GET", url="https://foo:443/bar", body=None, headers={'User-Agent': HTTP_USER_AGENT})
+            call(method="GET", url="https://foo:443/bar", body=None, headers={'User-Agent': HTTP_USER_AGENT, 'Connection': 'close'})
         ])
         self.assertEqual(1, mock_conn.getresponse.call_count)
         self.assertNotEquals(None, resp)