From dea9561af55cebecec261490eaaf09c26d53ad9f Mon Sep 17 00:00:00 2001 From: Ze Gan Date: Tue, 4 Jun 2024 01:06:59 +0800 Subject: [PATCH] [swig]: Fix swig template memory leak on issue 17025 (#876) (#878) Fix the issue sonic-net/sonic-buildimage#17025 about Redis set activity Description The issue reports a memory leak on the Redis set operations Reason Didn't decrease the reference count after PySequence_GetItem Use the inappropriate Swig API and didn't release the string of SWIG_AsPtr_std_string Fix: Refer PR: Fix swig template memory leak #859 from @praveenraja1 Replace the API SWIG_AsPtr_std_string to SWIG_AsVal_std_string Add unit test To monitor there is no dramatic memory increment after a huge amount of Redis set operations. Signed-off-by: Ze Gan --- .azure-pipelines/build-template.yml | 2 +- pyext/swsscommon.i | 39 ++++++++++++++++++----------- tests/test_redis_ut.py | 31 ++++++++++++++++++++++- 3 files changed, 56 insertions(+), 16 deletions(-) diff --git a/.azure-pipelines/build-template.yml b/.azure-pipelines/build-template.yml index f2a0ee496..5eebc1828 100644 --- a/.azure-pipelines/build-template.yml +++ b/.azure-pipelines/build-template.yml @@ -110,7 +110,7 @@ jobs: displayName: "Install gcovr 5.2 (for --exclude-throw-branches support)" - script: | set -ex - sudo pip install Pympler==0.8 + sudo pip install Pympler==0.8 pytest psutil sudo apt-get install -y redis-server sudo sed -i 's/notify-keyspace-events ""/notify-keyspace-events AKE/' /etc/redis/redis.conf sudo sed -ri 's/^# unixsocket/unixsocket/' /etc/redis/redis.conf diff --git a/pyext/swsscommon.i b/pyext/swsscommon.i index 6cab4d9e0..0b8d2b2fa 100644 --- a/pyext/swsscommon.i +++ b/pyext/swsscommon.i @@ -53,6 +53,8 @@ #include "zmqclient.h" #include "zmqconsumerstatetable.h" #include "zmqproducerstatetable.h" +#include +#include %} %include @@ -152,31 +154,36 @@ SWIG_Python_AppendOutput($result, temp); } -%typemap(in, fragment="SWIG_AsPtr_std_string") +%typemap(in, fragment="SWIG_AsVal_std_string") const std::vector,std::allocator< std::pair< std::string,std::string > > > & (std::vector< std::pair< std::string,std::string >,std::allocator< std::pair< std::string,std::string > > > temp, int res) { res = SWIG_OK; for (int i = 0; i < PySequence_Length($input); ++i) { temp.push_back(std::pair< std::string,std::string >()); - PyObject *item = PySequence_GetItem($input, i); - if (!PyTuple_Check(item) || PyTuple_Size(item) != 2) { + std::unique_ptr > item( + PySequence_GetItem($input, i), + [](PyObject *ptr){ + Py_DECREF(ptr); + }); + if (!PyTuple_Check(item.get()) || PyTuple_Size(item.get()) != 2) { SWIG_fail; } - PyObject *key = PyTuple_GetItem(item, 0); - PyObject *value = PyTuple_GetItem(item, 1); - std::string *ptr = (std::string *)0; + PyObject *key = PyTuple_GetItem(item.get(), 0); + PyObject *value = PyTuple_GetItem(item.get(), 1); + std::string str; + if (PyBytes_Check(key)) { temp.back().first.assign(PyBytes_AsString(key), PyBytes_Size(key)); - } else if (SWIG_AsPtr_std_string(key, &ptr)) { - temp.back().first = *ptr; + } else if (SWIG_AsVal_std_string(key, &str) != SWIG_ERROR) { + temp.back().first = str; } else { SWIG_fail; } if (PyBytes_Check(value)) { temp.back().second.assign(PyBytes_AsString(value), PyBytes_Size(value)); - } else if (SWIG_AsPtr_std_string(value, &ptr)) { - temp.back().second = *ptr; + } else if (SWIG_AsVal_std_string(value, &str) != SWIG_ERROR) { + temp.back().second = str; } else { SWIG_fail; } @@ -187,13 +194,17 @@ %typemap(typecheck) const std::vector< std::pair< std::string,std::string >,std::allocator< std::pair< std::string,std::string > > > &{ $1 = 1; for (int i = 0; i < PySequence_Length($input); ++i) { - PyObject *item = PySequence_GetItem($input, i); - if (!PyTuple_Check(item) || PyTuple_Size(item) != 2) { + std::unique_ptr > item( + PySequence_GetItem($input, i), + [](PyObject *ptr){ + Py_DECREF(ptr); + }); + if (!PyTuple_Check(item.get()) || PyTuple_Size(item.get()) != 2) { $1 = 0; break; } - PyObject *key = PyTuple_GetItem(item, 0); - PyObject *value = PyTuple_GetItem(item, 1); + PyObject *key = PyTuple_GetItem(item.get(), 0); + PyObject *value = PyTuple_GetItem(item.get(), 1); if (!PyBytes_Check(key) && !PyUnicode_Check(key) && !PyString_Check(key) diff --git a/tests/test_redis_ut.py b/tests/test_redis_ut.py index 2a8de621e..f0c219625 100644 --- a/tests/test_redis_ut.py +++ b/tests/test_redis_ut.py @@ -1,5 +1,6 @@ import os import time +import psutil import pytest import multiprocessing from threading import Thread @@ -801,4 +802,32 @@ def test_ConfigDBConnector(): allconfig["PORT_TABLE"] = None config_db.mod_config(allconfig) allconfig = config_db.get_config() - assert len(allconfig) == 0 \ No newline at end of file + assert len(allconfig) == 0 + + +def test_TableSetBinary(): + app_db = swsscommon.DBConnector("APPL_DB", 0, True) + t = swsscommon.Table(app_db, "TABLE") + buff = b"" + for i in range(0, 256): + buff += bytes([i]) + buff = buff.decode('latin-1') + fvs = swsscommon.FieldValuePairs([("binary", buff)]) + t.set("binary", fvs) + (status, fvs) = t.get("binary") + assert status == True + assert fvs[0][1] == buff + + +def test_TableOpsMemoryLeak(): + OP_COUNT = 50000 + app_db = swsscommon.DBConnector("APPL_DB", 0, True) + t = swsscommon.Table(app_db, "TABLE") + long_data = "x" * 100 + fvs = swsscommon.FieldValuePairs([(long_data, long_data)]) + rss = psutil.Process(os.getpid()).memory_info().rss + for _ in range(OP_COUNT): + t.set("long_data", fvs) + t.get("long_data") + assert psutil.Process(os.getpid()).memory_info().rss - rss < OP_COUNT +