fix: reduce memory overhead of .finalize() methods in PgConnection and StreamWrapper #2817

vlsi · 2023-02-18T12:33:04Z

Previously, an abandoned PgConnection could retain a significant amount of heap. Now the bare minimum is moved to a separate class, so the abandoned connections can be removed from the heap earlier.

This does not remove the use of .finalize(), and it only reduces the amount of retained heap memory.

vlsi · 2023-02-18T13:35:43Z

pgjdbc/src/main/java/org/postgresql/core/QueryExecutorCloseAction.java

+    // Prevent blocking the thread for too long
+    // The connection will be discarded anyway, so there's no much sense in waiting long
+    int timeout = pgStream.getNetworkTimeout();
+    if (timeout == 0 || timeout > 1000) {
+      pgStream.setNetworkTimeout(1000);
+    }
+    pgStream.sendChar('X');
+    pgStream.sendInteger4(4);
+
+    pgStream.flush();
+    pgStream.close();


Ideally, we should move this out of the finalizers thread, as it would get clogged if the application leaks more than one connection per second.
However, I would postpone that to a subsequent PR (e.g. move to PhantomReferences or something like that)

leaking more than 1 per second would only be a problem in face of networking issues causing us to hit the timeout, right?
In "happy path" of good connectivity to the database, sending the X4 should be much faster than that. Or am I missing something?

Imagine the user has something like:

for(i in 1..1000) { try {var con = openConnection();) { con.createStatement().execute(...); } } // assume the finalizer has not yet executed // assume the firewall kills the connections, and it blocks any data for them

In that case, the finalizer thread would have a pile of 1000 connections to clean up, and it would take ~1000 seconds to unclog while all the other finalizers won't be able to process.

Of course, it is a rare case, however, the finalizer thread is shared across all the other finalizers, so in the ideal world, we should refrain from blocking it even for 1 second.

Yeah - this is the horror scenario both in terms of volume of objects created, but also timing of network connectivity loss.

bokken · 2023-02-18T14:53:02Z

pgjdbc/src/main/java/org/postgresql/core/QueryExecutorCloseAction.java

+ * Implementation note: it should keep only the minimum number of object references
+ * to reduce heap usage in case the user abandons connection without closing it first.
+ */
+public class QueryExecutorCloseAction implements Closeable {


does this need to be public?
could this be final?

Previously, QueryExecutorBase.sendCloseMessage was open, so I need to keep this class open

pgjdbc/src/main/java/org/postgresql/core/QueryExecutorCloseAction.java

pgjdbc/src/main/java/org/postgresql/core/v3/QueryExecutorImpl.java

pgjdbc/src/main/java/org/postgresql/jdbc/PgConnectionFinalizeAction.java

pgjdbc/src/main/java/org/postgresql/util/StreamWrapper.java

pgjdbc/src/main/java/org/postgresql/util/StreamWrapperFinalizeAction.java

…d StreamWrapper Previously, an abandoned PgConnection could retain a significant amount of heap. Now the bare minimum is moved to a separate class, so the abandoned connections can be removed from the heap earlier. This does not remove the use of `.finalize()`, and it only reduces the amount of retained heap memory. Fixes pgjdbc#1431 Fixes pgjdbc#1360

vlsi force-pushed the reduce_finalize_overhead branch 7 times, most recently from 8324a12 to 52d0762 Compare February 18, 2023 13:23

This was referenced Feb 18, 2023

Memory leak in QueryExecutorImpl #1431

Closed

Finalizer thread causes application run out of heap #1360

Closed

vlsi force-pushed the reduce_finalize_overhead branch from 52d0762 to 1a12e09 Compare February 18, 2023 13:34

vlsi commented Feb 18, 2023

View reviewed changes