From 987a1a6167ed57a8a5b328c28acab06935b003cd Mon Sep 17 00:00:00 2001 From: Gilad Leifman Date: Sat, 8 Feb 2020 14:28:08 +0200 Subject: [PATCH 01/10] Updated .gitignore --- .gitignore | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/.gitignore b/.gitignore index fb9c43a6..a85e51e2 100644 --- a/.gitignore +++ b/.gitignore @@ -54,3 +54,14 @@ tags # The Access unit tests copy empty.accdb and empty.mdb to these names and use them. test.accdb test.mdb + +# JetBrain's IDE +.idea/ + +# Visual Studio IDE +.vs/ +*.sln +*.vcxproj* + +# Visual Studio build folder +x64/ \ No newline at end of file From 53e41c06dd6b8f0aa6ddf828d2468b0d1d7e8f5f Mon Sep 17 00:00:00 2001 From: Gilad Leifman Date: Sat, 8 Feb 2020 15:50:56 +0200 Subject: [PATCH 02/10] * Created a test file for the specific scenario --- tests3/dbapi_SQLParamData_memory__test.py | 91 +++++++++++++++++++ ...QLParamData_memory__test__requirements.txt | 1 + 2 files changed, 92 insertions(+) create mode 100644 tests3/dbapi_SQLParamData_memory__test.py create mode 100644 tests3/dbapi_SQLParamData_memory__test__requirements.txt diff --git a/tests3/dbapi_SQLParamData_memory__test.py b/tests3/dbapi_SQLParamData_memory__test.py new file mode 100644 index 00000000..2758fca8 --- /dev/null +++ b/tests3/dbapi_SQLParamData_memory__test.py @@ -0,0 +1,91 @@ +""" +This tests ensures that there is no memory leakage +after using SQLParamData that returns -1. + +One scenario where SQLParamData function will be used is when there is a parameterized +INSERT INTO query with at least one parameter's length is too big. +Note that In my case, 'too big' means pGetLen(pInfo->pObject) was more than 4000. + +In order to execute the INSERT INTO query, SQLExecute is used. +SQLExecute will return SQL_NEED_DATA (SQL_NEED_DATA = 99), +then SQLParamData will be used to create a SQL parameter and will return SQL_NEED_DATA. +After that SQLPutData will be used in a loop to save the data in this SQL parameter. +Then SQLParamData is called again, and if there is an error (-1), the data of newly +created SQL Parameter should be freed. + +This test should be tested against a table that has no space at all or no space in the +transaction log in order to get -1 value on the second call to SQLParamData. +The name of the table is stored in `TABLE_NAME`, change it to be your table's name. +""" +import os +import unittest + +import math +import psutil + +from tests3.testutils import add_to_path, load_setup_connection_string + +add_to_path() +import pyodbc + +KB = 1024 +MB = KB * 1024 + +ACCEPTABLE_MEMORY_DIFF = 500 * KB + +TABLE_NAME = "FullTable" + +CONNECTION_STRING = None + +CONNECTION_STRING_ERROR_MESSAGE = ( + r"Please create tmp\setup.cfg file or set a valid value to CONNECTION_STRING." +) + + +def current_total_memory_usage(): + """ + :return: Current total memory usage in bytes. + """ + process = psutil.Process(os.getpid()) + return process.memory_info().rss + + +class MemoryLeakSQLParamDataTestCase(unittest.TestCase): + driver = pyodbc + + @classmethod + def setUpClass(cls): + filename = os.path.splitext(os.path.basename(__file__))[0] + cls.connection_string = ( + load_setup_connection_string(filename) or CONNECTION_STRING + ) + + if not cls.connection_string: + return ValueError(CONNECTION_STRING_ERROR_MESSAGE) + + def test_memory_leak(self): + with pyodbc.connect(self.connection_string) as conn: + cursor = conn.cursor() + + current_memory_usage = current_total_memory_usage() + + query = "INSERT INTO {table_name} VALUES (?)".format(table_name=TABLE_NAME) + + try: + cur = cursor.execute(query, "a" * 10 * MB) + except self.driver.ProgrammingError as e: + self.assertEqual("42000", e.args[0]) + self.assertIn("SQLParamData", e.args[1]) + + after_excpetion_memory_usage = current_total_memory_usage() + + diff = math.fabs(after_excpetion_memory_usage - current_memory_usage) + self.assertLess(diff, ACCEPTABLE_MEMORY_DIFF) + + +def main(): + unittest.main() + + +if __name__ == "__main__": + main() diff --git a/tests3/dbapi_SQLParamData_memory__test__requirements.txt b/tests3/dbapi_SQLParamData_memory__test__requirements.txt new file mode 100644 index 00000000..0b574b52 --- /dev/null +++ b/tests3/dbapi_SQLParamData_memory__test__requirements.txt @@ -0,0 +1 @@ +psutil \ No newline at end of file From 5f3efe0769ccef3c4b2b5fbd0b0dafa9026c9dee Mon Sep 17 00:00:00 2001 From: Gilad Leifman Date: Sat, 8 Feb 2020 15:57:40 +0200 Subject: [PATCH 03/10] * Updated doc of test file for the specific SQLParamData scenario --- tests3/dbapi_SQLParamData_memory__test.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests3/dbapi_SQLParamData_memory__test.py b/tests3/dbapi_SQLParamData_memory__test.py index 2758fca8..d629aced 100644 --- a/tests3/dbapi_SQLParamData_memory__test.py +++ b/tests3/dbapi_SQLParamData_memory__test.py @@ -9,6 +9,7 @@ In order to execute the INSERT INTO query, SQLExecute is used. SQLExecute will return SQL_NEED_DATA (SQL_NEED_DATA = 99), then SQLParamData will be used to create a SQL parameter and will return SQL_NEED_DATA. +This call will create the pObject (PyObject) that should be freed. After that SQLPutData will be used in a loop to save the data in this SQL parameter. Then SQLParamData is called again, and if there is an error (-1), the data of newly created SQL Parameter should be freed. From 5e6473e3f95727104e8019a43b13369cdfb95817 Mon Sep 17 00:00:00 2001 From: Gilad Leifman Date: Sat, 8 Feb 2020 15:58:24 +0200 Subject: [PATCH 04/10] * Fixed the test file for the specific SQLParamData scenario by Py_XDECREF the PyObject with 1 reference. --- src/cursor.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/cursor.cpp b/src/cursor.cpp index d4686787..3b042f73 100644 --- a/src/cursor.cpp +++ b/src/cursor.cpp @@ -767,9 +767,15 @@ static PyObject* execute(Cursor* cur, PyObject* pSql, PyObject* params, bool ski ret = SQLParamData(cur->hstmt, (SQLPOINTER*)&pInfo); Py_END_ALLOW_THREADS - if (ret != SQL_NEED_DATA && ret != SQL_NO_DATA && !SQL_SUCCEEDED(ret)) - return RaiseErrorFromHandle(cur->cnxn, "SQLParamData", cur->cnxn->hdbc, cur->hstmt); + if (ret != SQL_NEED_DATA && ret != SQL_NO_DATA && !SQL_SUCCEEDED(ret)) + { + if (pInfo->allocated) + pyodbc_free(pInfo->ParameterValuePtr); + Py_XDECREF(pInfo->pObject); + return RaiseErrorFromHandle(cur->cnxn, "SQLParamData", cur->cnxn->hdbc, cur->hstmt); + } + TRACE("SQLParamData() --> %d\n", ret); if (ret == SQL_NEED_DATA) From f5b149fc67a815d9b02c8a363a328efa2b8559a2 Mon Sep 17 00:00:00 2001 From: Gilad Leifman Date: Sat, 8 Feb 2020 19:20:30 +0200 Subject: [PATCH 05/10] * Improved the test to close the cursor and set it to None, then forcing the gc --- tests3/dbapi_SQLParamData_memory__test.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/tests3/dbapi_SQLParamData_memory__test.py b/tests3/dbapi_SQLParamData_memory__test.py index d629aced..3fe11d49 100644 --- a/tests3/dbapi_SQLParamData_memory__test.py +++ b/tests3/dbapi_SQLParamData_memory__test.py @@ -18,6 +18,7 @@ transaction log in order to get -1 value on the second call to SQLParamData. The name of the table is stored in `TABLE_NAME`, change it to be your table's name. """ +import gc import os import unittest @@ -68,15 +69,19 @@ def test_memory_leak(self): with pyodbc.connect(self.connection_string) as conn: cursor = conn.cursor() - current_memory_usage = current_total_memory_usage() - query = "INSERT INTO {table_name} VALUES (?)".format(table_name=TABLE_NAME) + current_memory_usage = current_total_memory_usage() + try: cur = cursor.execute(query, "a" * 10 * MB) except self.driver.ProgrammingError as e: self.assertEqual("42000", e.args[0]) self.assertIn("SQLParamData", e.args[1]) + finally: + cursor.close() + cursor = None + gc.collect() after_excpetion_memory_usage = current_total_memory_usage() From 69f8b73d50b2ab2310e0e5c6eb3a97b1f0148da7 Mon Sep 17 00:00:00 2001 From: Gilad Leifman Date: Sat, 8 Feb 2020 19:59:51 +0200 Subject: [PATCH 06/10] * Changed the fix of the memory leak and updated the test. --- src/cursor.cpp | 11 +++-------- tests3/dbapi_SQLParamData_memory__test.py | 2 -- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/src/cursor.cpp b/src/cursor.cpp index 3b042f73..7922e59b 100644 --- a/src/cursor.cpp +++ b/src/cursor.cpp @@ -386,8 +386,8 @@ static void closeimpl(Cursor* cur) free_results(cur, FREE_STATEMENT | FREE_PREPARED); - FreeParameterInfo(cur); - FreeParameterData(cur); + FreeParameterData(cur); + FreeParameterInfo(cur); if (StatementIsValid(cur)) { @@ -768,13 +768,8 @@ static PyObject* execute(Cursor* cur, PyObject* pSql, PyObject* params, bool ski Py_END_ALLOW_THREADS if (ret != SQL_NEED_DATA && ret != SQL_NO_DATA && !SQL_SUCCEEDED(ret)) - { - if (pInfo->allocated) - pyodbc_free(pInfo->ParameterValuePtr); - Py_XDECREF(pInfo->pObject); - return RaiseErrorFromHandle(cur->cnxn, "SQLParamData", cur->cnxn->hdbc, cur->hstmt); - } + TRACE("SQLParamData() --> %d\n", ret); diff --git a/tests3/dbapi_SQLParamData_memory__test.py b/tests3/dbapi_SQLParamData_memory__test.py index 3fe11d49..eaaa29e6 100644 --- a/tests3/dbapi_SQLParamData_memory__test.py +++ b/tests3/dbapi_SQLParamData_memory__test.py @@ -80,8 +80,6 @@ def test_memory_leak(self): self.assertIn("SQLParamData", e.args[1]) finally: cursor.close() - cursor = None - gc.collect() after_excpetion_memory_usage = current_total_memory_usage() From 6195778eebedc6d7d6c0805ea1dc54370c24e820 Mon Sep 17 00:00:00 2001 From: Gilad Leifman Date: Sat, 8 Feb 2020 20:00:52 +0200 Subject: [PATCH 07/10] * Removed redundant empty line --- src/cursor.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/cursor.cpp b/src/cursor.cpp index 7922e59b..b04291e0 100644 --- a/src/cursor.cpp +++ b/src/cursor.cpp @@ -769,7 +769,6 @@ static PyObject* execute(Cursor* cur, PyObject* pSql, PyObject* params, bool ski if (ret != SQL_NEED_DATA && ret != SQL_NO_DATA && !SQL_SUCCEEDED(ret)) return RaiseErrorFromHandle(cur->cnxn, "SQLParamData", cur->cnxn->hdbc, cur->hstmt); - TRACE("SQLParamData() --> %d\n", ret); From b701c9f9655db0eda040e0b7d99d6cb771de460f Mon Sep 17 00:00:00 2001 From: Gilad Leifman Date: Sat, 8 Feb 2020 20:08:03 +0200 Subject: [PATCH 08/10] * Converted tabs to spaces --- src/cursor.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/cursor.cpp b/src/cursor.cpp index b04291e0..0e2c5981 100644 --- a/src/cursor.cpp +++ b/src/cursor.cpp @@ -386,8 +386,8 @@ static void closeimpl(Cursor* cur) free_results(cur, FREE_STATEMENT | FREE_PREPARED); - FreeParameterData(cur); - FreeParameterInfo(cur); + FreeParameterData(cur); + FreeParameterInfo(cur); if (StatementIsValid(cur)) { @@ -767,9 +767,9 @@ static PyObject* execute(Cursor* cur, PyObject* pSql, PyObject* params, bool ski ret = SQLParamData(cur->hstmt, (SQLPOINTER*)&pInfo); Py_END_ALLOW_THREADS - if (ret != SQL_NEED_DATA && ret != SQL_NO_DATA && !SQL_SUCCEEDED(ret)) - return RaiseErrorFromHandle(cur->cnxn, "SQLParamData", cur->cnxn->hdbc, cur->hstmt); - + if (ret != SQL_NEED_DATA && ret != SQL_NO_DATA && !SQL_SUCCEEDED(ret)) + return RaiseErrorFromHandle(cur->cnxn, "SQLParamData", cur->cnxn->hdbc, cur->hstmt); + TRACE("SQLParamData() --> %d\n", ret); if (ret == SQL_NEED_DATA) @@ -1061,9 +1061,9 @@ static PyObject* Cursor_executemany(PyObject* self, PyObject* args) if (cursor->fastexecmany) { free_results(cursor, FREE_STATEMENT | KEEP_PREPARED); - if (!ExecuteMulti(cursor, pSql, param_seq)) + if (!ExecuteMulti(cursor, pSql, param_seq)) return 0; - } + } else { for (Py_ssize_t i = 0; i < c; i++) From 1290726519fad64025e46439e850ccc7832d38ab Mon Sep 17 00:00:00 2001 From: Gilad Leifman Date: Sat, 8 Feb 2020 20:17:31 +0200 Subject: [PATCH 09/10] * Moved variable out of conn's scope --- tests3/dbapi_SQLParamData_memory__test.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests3/dbapi_SQLParamData_memory__test.py b/tests3/dbapi_SQLParamData_memory__test.py index eaaa29e6..0220c9bb 100644 --- a/tests3/dbapi_SQLParamData_memory__test.py +++ b/tests3/dbapi_SQLParamData_memory__test.py @@ -66,11 +66,11 @@ def setUpClass(cls): return ValueError(CONNECTION_STRING_ERROR_MESSAGE) def test_memory_leak(self): + query = "INSERT INTO {table_name} VALUES (?)".format(table_name=TABLE_NAME) + with pyodbc.connect(self.connection_string) as conn: cursor = conn.cursor() - query = "INSERT INTO {table_name} VALUES (?)".format(table_name=TABLE_NAME) - current_memory_usage = current_total_memory_usage() try: From 850a8dde93a0788c7556c1c7653c8c1ff838101a Mon Sep 17 00:00:00 2001 From: Gilad Leifman Date: Fri, 22 Jan 2021 18:00:56 +0200 Subject: [PATCH 10/10] Update gitignore, remove duplicated --- .gitignore | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/.gitignore b/.gitignore index a85e51e2..d153bb85 100644 --- a/.gitignore +++ b/.gitignore @@ -55,13 +55,3 @@ tags test.accdb test.mdb -# JetBrain's IDE -.idea/ - -# Visual Studio IDE -.vs/ -*.sln -*.vcxproj* - -# Visual Studio build folder -x64/ \ No newline at end of file