Fix memory leak when query fails with permission error #12270

viczhang861 · 2019-01-28T20:26:54Z

When permission error is thrown inside creation of QueryExecution object,
transaction metadata is not marked as inactive resulting in memory leak.

Existing handling of error via method TransactionMetadata::fail is not
working when transaction id is empty in session object.

A heapdump is taken to test that previously leaked objects become
unreachable from GC roots after this fix.

elonazoulay

Looks good, can you add a test?

arhimondr

@picrinite Looks good, thanks for figuring this out.

@elonazoulay Yeah, i think we should add a test. I think what we can try to do, is to create a new integration test class, create a distributed query runner, run different scenarios upon that query runner when checking that transaction manager is empty after every scenario.

arhimondr · 2019-01-29T15:17:17Z

presto-main/src/main/java/com/facebook/presto/execution/QueryStateMachine.java

+        queryStateMachine.addStateChangeListener(newState -> {
+            QUERY_STATE_LOG.debug("Query %s is %s", queryStateMachine.getQueryId(), newState);
+            // mark finished or failed transaction as inactive
+            if (newState.isDone()) {


@picrinite Did you check that this listener is triggered when the analysis fails (the case with permission error)?

The last time we discussed it, we were thinking about wrapping the analysis into a try-catch, because we thought that the state change listener may not always be triggered. Is that not the case?

@arhimondr The first commit is an improvement to make it safer for "setInactive" job. It is not triggered for permission error.
The second commit guarantees "setInactive" job is triggered when there is permission error, but there are some tests failed and I don't know how to reproduce. Help appreciated !
Regarding automatic test, cleanup of transaction manager is an asynchronous job with a delay (current default is 5 minutes), it is not deterministic.

An integration test is added, the added test will fail without current fix.

presto-main/src/main/java/com/facebook/presto/execution/SqlQueryExecution.java

nezihyigitbasi

Merge StateChangeListener for QueryStateMachine

IIUC this commit just moves the listener registration to QueryStateMachine right after the creation of the state machine. If this is correct we can update the commit title as Register state change listener in QueryStateMachine or sth similar.

nezihyigitbasi

Fix memory leak when query fails with permission error

The commit message talks about permission error only, but the fix is actually a generic fix, so I think we can make the commit title more generic.

nezihyigitbasi · 2019-02-04T20:48:48Z

presto-main/src/main/java/com/facebook/presto/execution/SqlQueryManager.java

@@ -496,6 +489,11 @@ public ThreadPoolExecutorMBean getManagementExecutor()
        return queryManagementExecutorMBean;
    }

+    public TransactionManager getTransactionManager()


Annotate with @VisibleForTesting.

nezihyigitbasi · 2019-02-04T20:49:30Z

presto-tests/src/test/java/com/facebook/presto/tests/TestQueryManager.java

@@ -56,6 +60,30 @@ public void tearDown()
        queryRunner = null;
    }

+    @Test(timeOut = 100_000L)


Why not 60s? 100s is a bit on the long side.

nezihyigitbasi · 2019-02-04T20:50:18Z

presto-tests/src/test/java/com/facebook/presto/tests/TestQueryManager.java

+        QueryInfo queryInfo = sqlQueryManager.getFullQueryInfo(queryId);
+        assertEquals(queryInfo.getState(), FAILED);
+        assertNotNull(queryInfo.getFailureInfo());
+        TimeUnit.MILLISECONDS.sleep(50);


Why is there a sleep call here? Does the correctness of the test depend on this call?

Can we get a future on that metadata cleanup asyn job and wait until it's done? Sleeping is fragile, this test can fail arbitrarily depending on the interleaving of the async job and the thread running this test.

@nezihyigitbasi After further investigation with @arhimondr, we confirmed sleep() is not necessary since cleanup is synchronous. I have removed it from test.

Register transaction cleanup job immediatelly after QueryStateMachine object is created to avoid failure.

arhimondr

LGTM % comment

presto-tests/src/test/java/com/facebook/presto/tests/TestQueryManager.java

presto-tests/src/test/java/com/facebook/presto/tests/TestTransactionManagerIntegration.java

When semantic error (including permission error) is thrown during creation of SqlQueryExecution object, transaction metadata is not cleaned up resulting in memory leak. Current handling of error via method TransactionMetadata::fail is not working if there is no existing transaction id in session object.

Summary: Pull Request resolved: facebookincubator/velox#12270 Differential Revision: D69208299

viczhang861 self-assigned this Jan 28, 2019

viczhang861 requested review from wenleix, elonazoulay, arhimondr and mbasmanova January 28, 2019 20:26

facebook-github-bot added the CLA Signed label Jan 28, 2019

mbasmanova requested review from wenleix and removed request for wenleix January 28, 2019 20:31

viczhang861 changed the title ~~Fix memory leak when query fails with permission error~~ [WIP] Fix memory leak when query fails with permission error Jan 28, 2019

viczhang861 force-pushed the cleanFailedQuery branch from edfc5c1 to ade4c5a Compare January 28, 2019 23:40

elonazoulay suggested changes Jan 29, 2019

View reviewed changes

arhimondr reviewed Jan 29, 2019

View reviewed changes

presto-main/src/main/java/com/facebook/presto/execution/SqlQueryExecution.java Outdated Show resolved Hide resolved

viczhang861 force-pushed the cleanFailedQuery branch 2 times, most recently from 3efda78 to bced22d Compare January 29, 2019 21:06

viczhang861 changed the title ~~[WIP] Fix memory leak when query fails with permission error~~ Fix memory leak when query fails with permission error Jan 29, 2019

viczhang861 force-pushed the cleanFailedQuery branch 2 times, most recently from d287858 to 28d7fbf Compare February 4, 2019 18:29

nezihyigitbasi reviewed Feb 4, 2019

View reviewed changes

viczhang861 force-pushed the cleanFailedQuery branch from 28d7fbf to ebfb3d1 Compare February 5, 2019 16:56

Register state change listener to expire transaction

d4776a2

Register transaction cleanup job immediatelly after QueryStateMachine object is created to avoid failure.

viczhang861 force-pushed the cleanFailedQuery branch from ebfb3d1 to 75f7343 Compare February 7, 2019 02:48

arhimondr approved these changes Feb 7, 2019

View reviewed changes

presto-tests/src/test/java/com/facebook/presto/tests/TestQueryManager.java Outdated Show resolved Hide resolved

presto-tests/src/test/java/com/facebook/presto/tests/TestTransactionManagerIntegration.java Show resolved Hide resolved

viczhang861 force-pushed the cleanFailedQuery branch from 75f7343 to 2d01d12 Compare February 7, 2019 23:06

arhimondr approved these changes Feb 8, 2019

View reviewed changes

presto-tests/src/test/java/com/facebook/presto/tests/TestTransactionManagerIntegration.java Show resolved Hide resolved

viczhang861 force-pushed the cleanFailedQuery branch from 2d01d12 to b858007 Compare February 8, 2019 22:13

viczhang861 merged commit b7d3c85 into prestodb:master Feb 25, 2019

viczhang861 deleted the cleanFailedQuery branch February 25, 2019 18:57

viczhang861 restored the cleanFailedQuery branch February 26, 2019 23:04

viczhang861 deleted the cleanFailedQuery branch March 4, 2019 15:52

natashasehgal added a commit to natashasehgal/presto that referenced this pull request Feb 5, 2025

Add TDigest type and vabinary conversion (prestodb#12270)

fb51487

Summary: Pull Request resolved: facebookincubator/velox#12270 Differential Revision: D69208299

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix memory leak when query fails with permission error #12270

Fix memory leak when query fails with permission error #12270

viczhang861 commented Jan 28, 2019 •

edited

Loading

elonazoulay left a comment

arhimondr left a comment •

edited

Loading

arhimondr Jan 29, 2019

viczhang861 Jan 29, 2019

viczhang861 Feb 4, 2019

nezihyigitbasi left a comment

nezihyigitbasi left a comment

nezihyigitbasi Feb 4, 2019

nezihyigitbasi Feb 4, 2019

nezihyigitbasi Feb 4, 2019

nezihyigitbasi Feb 5, 2019

viczhang861 Feb 7, 2019

arhimondr left a comment

Fix memory leak when query fails with permission error #12270

Fix memory leak when query fails with permission error #12270

Conversation

viczhang861 commented Jan 28, 2019 • edited Loading

elonazoulay left a comment

Choose a reason for hiding this comment

arhimondr left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nezihyigitbasi left a comment

Choose a reason for hiding this comment

nezihyigitbasi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arhimondr left a comment

Choose a reason for hiding this comment

viczhang861 commented Jan 28, 2019 •

edited

Loading

arhimondr left a comment •

edited

Loading