Skip to content

fix possible resource leak on PostgreSQL access#208

Closed
SanshiroEnomoto wants to merge 1 commit into
developfrom
pgsql-resource-leak-fix
Closed

fix possible resource leak on PostgreSQL access#208
SanshiroEnomoto wants to merge 1 commit into
developfrom
pgsql-resource-leak-fix

Conversation

@SanshiroEnomoto

Copy link
Copy Markdown

Fixes

The conn.close() method must be called to avoid resource leakage. This is automatically done when the conn variable is deleted (we should not rely on this, though), and often the conn variable is automatically deleted when returned from the function; however, if an exception happens, all the local variables are capcured by the traceback object and raised to the caller, causing the garbage collector not removing it.

We actually observed resource leakage that led to reliable crashes after 90 min when exceptions are constantly thrown, and the crashing stopped after stopping the exception (writing data before the endpoint entry is made on the name-id map). It is not clear yet why the traceback objects are not deleted on the next exception, but given the observations, doing this good practice (explicit resource management) would be a good thing anyway.

Testing

I didn't run any test. I did not even run the updated script.

Prior to merging for releases:

  • update the project's version in the top-level CMakeLists.txt file
  • update the appVersion to be the new container image tag version in chart/Chart.yaml

- The conn.close() method must be called to avoid resource leakage.
- This is automatically done when the conn variable is deleted (we should not rely on this, though).
- Often the conn variable is automatically deleted when returned from the function; however,
- if an exception happens, all the local variables are capcured by the traceback object and raised to the caller,
- causing the garbage collector not removing it.

- We actually obverved resource leakage that led to reliable crashes after 90 min when exceptions are constantly thrown,
- and the crashing stopped after stopping the exception (writing data before the endpoint entry is made on the mane-id map).
- It is not clear yet why the traceback objects are not deleted on the next exception,
- but given the observations, doing this good practice (explicit resource management) would be a good thing anyway.
@wcpettus
wcpettus changed the base branch from main to develop July 28, 2025 15:55
@SanshiroEnomoto
SanshiroEnomoto requested a review from nsoblath July 28, 2025 16:54
@wcpettus

Copy link
Copy Markdown
Contributor

I think this is already resolved on the develop branch (as of #200), and should be implemented in the next dl-py release

dripline-python$ git diff pgsql-resource-leak-fix..develop dripline/implementations/postgres_interface.py 
diff --git a/dripline/implementations/postgres_interface.py b/dripline/implementations/postgres_interface.py
index 4b54866..b2095cf 100644
--- a/dripline/implementations/postgres_interface.py
+++ b/dripline/implementations/postgres_interface.py
@@ -131,10 +131,9 @@ class SQLTable(Endpoint):
         Returns: a tuple, 1st element is list of column names, 2nd is a list of tuples of the rows that matched the select
         '''
         if not return_cols:
-            return_cols = self.table.c
+            this_select = sqlalchemy.select(self.table)
         else:
-            return_cols = [sqlalchemy.text(col) for col in return_cols]
-        this_select = sqlalchemy.select(return_cols)
+            this_select = sqlalchemy.select(*[getattr(self.table.c,col) for col in return_cols])
         for c,v in where_eq_dict.items():
             this_select = this_select.where(getattr(self.table.c,c)==v)
         for c,v in where_lt_dict.items():

The "Files Changed" tab is useless because the git histories have diverged, so @SanshiroEnomoto please verify.

@SanshiroEnomoto

Copy link
Copy Markdown
Author

Yes, the PL #200 includes the same changes.

@nsoblath

nsoblath commented Aug 7, 2025

Copy link
Copy Markdown
Member

Closing this PR since the changes are already implemented from another PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants