[HFWBmuq2] Fixes #155: Check for correct tx terminations #256

vga91 · 2022-11-28T07:07:20Z

Fixes #155

Added terminationGuard.check() where needed.
Added various tests via TransactionTestUtil to verify procedures/functions are correctly terminated when the transaction is interrupted.
Changed movies.cypher in order to be executed multiple times in BigGraphTest

gem-neo4j · 2022-11-29T09:23:41Z

common/src/main/resources/movies.cypher

-CREATE (AndyW:Person {name:'Andy Wachowski', born:1967})
-CREATE (LanaW:Person {name:'Lana Wachowski', born:1965})
-CREATE (JoelS:Person {name:'Joel Silver', born:1952})
+CREATE (Keanu:Person:Other {name:'Keanu Reeves', id: randomUUID(), born:1964})


This is the same file used for apoc.examples.movies? we should perhaps not change it if it is as it would be a breaking change I imagine, perhaps make a new file for testing purposes instead

Oh, i didn't noticed it :( Now I created another file

gem-neo4j · 2022-11-29T09:26:13Z

core/src/main/java/apoc/cypher/Timeboxed.java

@@ -52,6 +57,12 @@ public Stream<MapResult> runTimeboxed(@Name("statement") String cypher, @Name("p
                txAtomic.set(innerTx);
                Result result = innerTx.execute(cypher, params == null ? Collections.EMPTY_MAP : params);
                while (result.hasNext()) {
+                    if (Util.transactionIsTerminated(terminationGuard)) {
+                        txAtomic.get().close();
+                        offerToQueue(queue, POISON, timeout);


I don't really understand the Posion part here, if the tx is terminated would we not want to just return the same warning as below? I might be missing some context here, so feel free to explain it to me :)

A bit strange but without the poison queue, the transaction is marked as terminated but actually continues the execution until it finishes, and after that throws an exception The transaction has been terminated...

Anyway, I realized that the TransactionTestUtil.checkTerminationGuard could potentially produce a false positive, therefore I also added the execution time check (which in fact fails without this POISON)

gem-neo4j · 2022-11-29T09:28:38Z

core/src/main/java/apoc/load/Xml.java

@@ -190,6 +194,7 @@ private boolean proceedReader(XMLStreamReader reader) throws XMLStreamException
    }

    private void handleNode(Deque<Map<String, Object>> stack, Node node, boolean simpleMode) {
+        terminationGuard.check();


If this check fails does it throw an exception? do we need to do anything, or just let it happen :) perhaps a comment saying what it does could be useful

Yes it throw an exception if the check fails.
Do you mean add a comment for each piece of code with terminationGuard.check()?

However I personally don't think if it's really necessary add a comment, because it's already explained in the org.neo4j.procedure.TerminationGuard interface

Oh I didn't see the comment in the interface! This is fine then, thanks for explaining :)

gem-neo4j · 2022-11-29T09:30:36Z

core/src/test/java/apoc/cypher/CypherTest.java

+        // assert query terminated (RETURN 0)
+        TestUtil.testCall(db, query,
+                Map.of("innerQuery", innerLongQuery),
+                row -> assertEquals(Map.of("0", 0L), row.get("value")));


Is there a way to check warnings returned maybe? (if there is one returned that is)

In this case the log.warn("query " + cypher + " has been terminated") is not returned, because we terminate the inner transaction, so it doesn't execute that piece of code

Ah I see, thanks :)

and afaik there is no way for procedures to add a warning to the Cypher ExecutionContext

gem-neo4j · 2022-11-29T09:38:51Z

core/src/test/java/apoc/periodic/PeriodicTest.java

+                        final Object actual = ((Map) row.get("updateStatistics")).get("nodesCreated");
+                        assertEquals(0L, actual);
+                    });
+            fail("Should have terminated");


What does this fail give us?

It is just to verify that the query really fails, because of terminationGuard, and don't go ahead without errors.

gem-neo4j · 2022-11-29T09:39:28Z

test-utils/src/main/java/apoc/periodic/PeriodicTestUtils.java

@@ -41,8 +42,19 @@ public static boolean terminateQuery(String pattern, GraphDatabaseAPI db) {
        return numberOfKilledTransactions > 0;
    }

+    // todo - we could get rid of this method (and related code), and leverage the TransactionTestUtil.testTerminateWithCommand


I'd prefer todos are added as Trello tickets instead of in the code :)

I added the trello ticket, id aWY2zoX5 in Low priority column,
but I can always move it elsewhere, in case :)

Awesome, thanks

gem-neo4j · 2022-11-29T09:40:48Z

test-utils/src/main/java/apoc/util/TransactionTestUtil.java

+                            return false;
+                        }
+                        transactionId[0] = msgIterator.next();
+                        return transactionId[0] != null;


Why is the transactionId an array and not just a string?

gem-neo4j · 2022-11-29T09:41:34Z

core/src/test/java/apoc/load/LoadJsonTest.java

+
+    @Test
+    public void shouldTerminateLoadWhenTransactionIsTimedOut() {
+        final String query = "CALL apoc.load.json('https://github.com/knowitall/yelp-dataset-challenge/blob/master/data/yelp_phoenix_academic_dataset/yelp_academic_dataset_review.json?raw=truehttps://github.com/knowitall/yelp-dataset-challenge/blob/master/data/yelp_phoenix_academic_dataset/yelp_academic_dataset_review.json?raw=true')";


I'd prefer you didn't reference external resources in case it is deleted in the future, is it possible to add to this repo instead?

gem-neo4j

Nice work with this :)

jexp · 2022-12-15T18:11:14Z

@gem-neo4j are we good to merge this one?

gem-neo4j · 2022-12-16T07:34:38Z

@jexp yep :) after a green build it will be

CLAassistant · 2023-01-24T09:24:20Z

All committers have signed the CLA.

…eo4j/apoc#256) * [HFWBmuq2] Fixes neo4j/apoc#155: Check for correct tx terminations * [HFWBmuq2] Code clean * [HFWBmuq2] removed unused code * [HFWBmuq2] try solving flaky tests * [HFWBmuq2] changes review - added time check * [HFWBmuq2] added local file tests - small changes * [HFWBmuq2] removed unused imports after rebase * [HFWBmuq2] fix flaky transaction not found error * [HFWBmuq2] Fix heapspace and flaky errors

…eo4j/apoc#256) (#3534) * [HFWBmuq2] Fixes neo4j/apoc#155: Check for correct tx terminations (neo4j/apoc#256) * [HFWBmuq2] Fixes neo4j/apoc#155: Check for correct tx terminations * [HFWBmuq2] Code clean * [HFWBmuq2] removed unused code * [HFWBmuq2] try solving flaky tests * [HFWBmuq2] changes review - added time check * [HFWBmuq2] added local file tests - small changes * [HFWBmuq2] removed unused imports after rebase * [HFWBmuq2] fix flaky transaction not found error * [HFWBmuq2] Fix heapspace and flaky errors * [HFWBmuq2] [Bc2lkk3N] Fixed testImportCsvTerminate and added TerminationGuard to apoc.import.csv (neo4j/apoc#343) * [Bc2lkk3N] Fix testImportCsvTerminate and add TerminationGuard to apoc.import.csv * [Bc2lkk3N] Fix failing tests * [Bc2lkk3N] Fix typo * [HFWBmuq2] fix compile error * [HFWBmuq2] remove imports * [HFWBmuq2] fix flaky Timeboxed error * [HFWBmuq2] removed unused imports

…eo4j/apoc#256) (neo4j-contrib#3534) * [HFWBmuq2] Fixes neo4j/apoc#155: Check for correct tx terminations (neo4j/apoc#256) * [HFWBmuq2] Fixes neo4j/apoc#155: Check for correct tx terminations * [HFWBmuq2] Code clean * [HFWBmuq2] removed unused code * [HFWBmuq2] try solving flaky tests * [HFWBmuq2] changes review - added time check * [HFWBmuq2] added local file tests - small changes * [HFWBmuq2] removed unused imports after rebase * [HFWBmuq2] fix flaky transaction not found error * [HFWBmuq2] Fix heapspace and flaky errors * [HFWBmuq2] [Bc2lkk3N] Fixed testImportCsvTerminate and added TerminationGuard to apoc.import.csv (neo4j/apoc#343) * [Bc2lkk3N] Fix testImportCsvTerminate and add TerminationGuard to apoc.import.csv * [Bc2lkk3N] Fix failing tests * [Bc2lkk3N] Fix typo * [HFWBmuq2] fix compile error * [HFWBmuq2] remove imports * [HFWBmuq2] fix flaky Timeboxed error * [HFWBmuq2] removed unused imports

…eo4j/apoc#256) (#3534) * [HFWBmuq2] Fixes neo4j/apoc#155: Check for correct tx terminations (neo4j/apoc#256) * [HFWBmuq2] Fixes neo4j/apoc#155: Check for correct tx terminations * [HFWBmuq2] Code clean * [HFWBmuq2] removed unused code * [HFWBmuq2] try solving flaky tests * [HFWBmuq2] changes review - added time check * [HFWBmuq2] added local file tests - small changes * [HFWBmuq2] removed unused imports after rebase * [HFWBmuq2] fix flaky transaction not found error * [HFWBmuq2] Fix heapspace and flaky errors * [HFWBmuq2] [Bc2lkk3N] Fixed testImportCsvTerminate and added TerminationGuard to apoc.import.csv (neo4j/apoc#343) * [Bc2lkk3N] Fix testImportCsvTerminate and add TerminationGuard to apoc.import.csv * [Bc2lkk3N] Fix failing tests * [Bc2lkk3N] Fix typo * [HFWBmuq2] fix compile error * [HFWBmuq2] remove imports * [HFWBmuq2] fix flaky Timeboxed error * [HFWBmuq2] removed unused imports

vga91 force-pushed the issue_155 branch from de29fc8 to c22043b Compare November 28, 2022 08:37

Lojjs assigned gem-neo4j Nov 28, 2022

vga91 force-pushed the issue_155 branch from cecdc3c to 4786ffb Compare November 28, 2022 13:12

gem-neo4j reviewed Nov 29, 2022

View reviewed changes

gem-neo4j approved these changes Dec 5, 2022

View reviewed changes

vga91 force-pushed the issue_155 branch 2 times, most recently from 3d226f2 to 9622843 Compare December 15, 2022 15:18

vga91 force-pushed the issue_155 branch 3 times, most recently from 0a919cf to c24687b Compare January 5, 2023 16:29

vga91 force-pushed the issue_155 branch 3 times, most recently from 9e813d9 to 7dc7d9a Compare January 17, 2023 14:11

vga91 force-pushed the issue_155 branch 2 times, most recently from 53907c8 to a008e94 Compare January 23, 2023 08:26

vga91 force-pushed the issue_155 branch from 140d591 to e1671e7 Compare January 24, 2023 13:24

vga91 added 4 commits February 21, 2023 16:22

[HFWBmuq2] Fixes #155: Check for correct tx terminations

303f8d5

[HFWBmuq2] Code clean

00d29b8

[HFWBmuq2] removed unused code

bf327e2

[HFWBmuq2] try solving flaky tests

a23bbd2

vga91 added 4 commits February 21, 2023 16:22

[HFWBmuq2] changes review - added time check

a85574e

[HFWBmuq2] added local file tests - small changes

8a6d220

[HFWBmuq2] removed unused imports after rebase

76797af

[HFWBmuq2] fix flaky transaction not found error

adcff02

vga91 force-pushed the issue_155 branch 4 times, most recently from 0fc7e82 to b15b3ad Compare February 22, 2023 13:10

[HFWBmuq2] Fix heapspace and flaky errors

9230d8c

vga91 force-pushed the issue_155 branch from b15b3ad to 9230d8c Compare February 23, 2023 17:28

vga91 merged commit ab6d58a into dev Feb 27, 2023

vga91 deleted the issue_155 branch February 27, 2023 08:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HFWBmuq2] Fixes #155: Check for correct tx terminations #256

[HFWBmuq2] Fixes #155: Check for correct tx terminations #256

vga91 commented Nov 28, 2022

gem-neo4j Nov 29, 2022

vga91 Nov 29, 2022

gem-neo4j Nov 29, 2022

vga91 Nov 29, 2022

gem-neo4j Nov 29, 2022

vga91 Nov 29, 2022

gem-neo4j Dec 1, 2022

gem-neo4j Nov 29, 2022

vga91 Nov 29, 2022

gem-neo4j Dec 1, 2022

jexp Dec 15, 2022

gem-neo4j Nov 29, 2022

vga91 Nov 29, 2022

gem-neo4j Nov 29, 2022

vga91 Nov 29, 2022

gem-neo4j Dec 1, 2022

gem-neo4j Nov 29, 2022

gem-neo4j Nov 29, 2022

gem-neo4j left a comment

jexp commented Dec 15, 2022

gem-neo4j commented Dec 16, 2022

CLAassistant commented Jan 24, 2023 •

edited

Loading

[HFWBmuq2] Fixes #155: Check for correct tx terminations #256

[HFWBmuq2] Fixes #155: Check for correct tx terminations #256

Conversation

vga91 commented Nov 28, 2022

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

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

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

gem-neo4j left a comment

Choose a reason for hiding this comment

jexp commented Dec 15, 2022

gem-neo4j commented Dec 16, 2022

CLAassistant commented Jan 24, 2023 • edited Loading

CLAassistant commented Jan 24, 2023 •

edited

Loading