Project

General

Profile

Bug #10083

Infinite loop when changing walking index fields

Added by Eduard Soltan about 1 year ago. Updated about 1 year ago.

Status:
Test
Priority:
Normal
Assignee:
Eduard Soltan
Target version:
-
Start date:
Due date:
% Done:

100%

billable:
No
vendor_id:
GCD
case_num:
version_reported:
version_resolved:
reviewer:
Tomasz Domin
production:
No
env_name:
topics:

fwd.df (473 Bytes) Eduard Soltan, 06/02/2025 04:38 PM

zfile_set.txt Magnifier (586 Bytes) Tomasz Domin, 06/05/2025 05:53 AM

Related issues

Related to Database - Bug #10390: FIND FIRST buffer field change does not propagate into the next FIRST FIRST statements. Internal Test

History

#1 Updated by Eduard Soltan about 1 year ago

Manage to create a small test case for infinite loop in unit tests in customer application. Please also look at the .df file.

def buffer buf1 for book1.

for each book1:
    delete book1.
end.

create book1.
book1.f1 = 1.
book1.f2 = 3.
release book1.

do transaction:
   find first book1 where book1.f1 = 1 and book1.f2 = 3.

   book1.f2 = 0.

   find first buf1 where buf1.f1 = 1 and buf1.f2 = 3.
   message buf1.f1 buf1.f2.
end.

Note: In OE I will get an ** FIND FIRST/LAST failed for table buf1 at second find.

What happens in fwd:

Lets suppose that the record in book1 table with (f1 = 1 and f2 = 3) has the recid = 100.

After the update of book1.f2 = 0, the record in the database is still with (f1 = 1 and f2 = 3). But in Session cache we put a new record with (f1 = 1 and f2 = 0). Also in DirtySharedDatabase a new record it is inserted the (f1 = 1 and f2 = 0).

RandomAccessQuery.executeImpl
On the the second FIND FIRST with the where clause where f1 = 1 and f2 = 3, on first try it will fetch the record from the database, but replace it with the Session cache version.

Next step is to get the record with the same where clause from the DirtyDatabase. Obviously it will not find any matching the record in DD, but will not find the a entry in the changed set with the recid of the previously find record. So from DirtySharedContext I will receive { dirtyDMO: null, changed: true }.

Next step is to execute the DirtyShared logic from RAQ.executeImpl. But because the dirtyDMO is null, it will just move to the next iteration.
When activeBundleKey is FIRST/LAST, it will changed the activeBandle which could give any value at the next iteration.
But when activeBundleKey is NEXT/PREVIOUS, the next iteration will not give anything of value.

Solution

My proposed solution is to break of loop in this situation:

=== modified file 'src/com/goldencode/p2j/persist/RandomAccessQuery.java'
--- old/src/com/goldencode/p2j/persist/RandomAccessQuery.java    2025-05-13 07:25:00 +0000
+++ new/src/com/goldencode/p2j/persist/RandomAccessQuery.java    2025-06-02 18:35:16 +0000
@@ -5808,7 +5808,7 @@
                   case FIRST:
                      activateNext();
                      break;
-                  case LAST:
+                  case LAST: 
                      activatePrevious();
                      break;
                }
@@ -5819,6 +5819,12 @@
                placeholder = dmo;
                dmo = null;

+               if (activeBundleKey == NEXT ||
+                   activeBundleKey == PREVIOUS)
+               {
+                  break;
+               }
+               
                // ...then try another pass.
                continue;
             }

#3 Updated by Eduard Soltan about 1 year ago

  • Subject changed from Infinite loop when changing walking table index to Infinite loop when changing walking index fields

#4 Updated by Eduard Soltan about 1 year ago

  • Status changed from New to WIP

With the patch from #10083-1, unit tests of a customer application does not get stuck in a infinite loop. Committed my solution on 10083a, rev. 15960.

#5 Updated by Eduard Soltan about 1 year ago

  • Status changed from WIP to Review
  • % Done changed from 0 to 100

#6 Updated by Eduard Soltan about 1 year ago

  • reviewer Tomasz Domin added

#7 Updated by Tomasz Domin about 1 year ago

I am not sure, but I think I made similar change in past and there was a problem. Let me check it with testcases/dirty share tests.

#8 Updated by Tomasz Domin about 1 year ago

I've run support/harness/test/cross_session_test.xml dirty share tests and they fail much more compared to trunk.
On trunk only one test fails due #9615 changes - which is wrong, but acceptable as no workaround for problem is known.
But when running with 10083a five more tests fail.

1. test19LeakOnFindByRecordid: Test failure:org.opentest4j.AssertionFailedError:Expected: pi-2|si1-2|si2-2|uf-2 but was: ?
2. test20LeakOnRelease: Test failure:org.opentest4j.AssertionFailedError:Expected: pi-3|si1-3|si2-1|uf-6 but was: ? <- that is caused by #9615
3. test15LeakPreviousOnSecondaryIndexUpdate: Test failure:org.opentest4j.AssertionFailedError:Expected: pi-2|si1-2|si2-2|uf-2 but was: ?
4. test17NoLeadOnUnindexedFieldUpdate: Test failure:org.opentest4j.AssertionFailedError:Expected: pi-2|si1-2|si2-2|uf-2 but was: ?
5. test18LeakOnFindNotEquals: Test failure:org.opentest4j.AssertionFailedError:Expected: pi-2|si1-2|si2-2|uf-2 but was: ?
6. test16LeakCurrentOnFindNotEqual: Test failure:org.opentest4j.AssertionFailedError:Expected: pi-2|si1-2|si2-2|uf-2 but was: ?

The problem with the change is that looking for the next/previous record prematurely ends. The next iteration would try to get record from intra-session dirty share.

#9 Updated by Eduard Soltan about 1 year ago

Is there a wiki to run support/harness/test/cross_session_test.xml?. I tried to run them myself it was giving me all kind of errors.

#10 Updated by Tomasz Domin about 1 year ago

No wiki yet.
See #9270, #6860#80, support/harness/readme.txt (that will go into Wiki)
Attached zfile_set.txt for given tests to run.

#11 Updated by Eduard Soltan about 1 year ago

Tomasz Domin wrote:

No wiki yet.
See #9270, #6860#80, support/harness/readme.txt (that will go into Wiki)
Attached zfile_set.txt for given tests to run.

But can I enter in the debugger in the culprit test class, would the issue still be reproduced?

#12 Updated by Eduard Soltan about 1 year ago

Tomasz Domin wrote:

I've run support/harness/test/cross_session_test.xml dirty share tests and they fail much more compared to trunk.
On trunk only one test fails due #9615 changes - which is wrong, but acceptable as no workaround for problem is known.
But when running with 10083a five more tests fail.
[...]
The problem with the change is that looking for the next/previous record prematurely ends. The next iteration would try to get record from intra-session dirty share.

Ok, but if we break out of the loop on next iteration if it enters the same part of code?

#13 Updated by Eduard Soltan about 1 year ago 

=== modified file 'src/com/goldencode/p2j/persist/RandomAccessQuery.java'
--- old/src/com/goldencode/p2j/persist/RandomAccessQuery.java    2024-11-11 12:58:50 +0000
+++ new/src/com/goldencode/p2j/persist/RandomAccessQuery.java    2025-06-05 13:50:36 +0000
@@ -5475,6 +5475,8 @@
          boolean needsLock = updateLock && lockType != LockType.NONE;
          boolean findByRowid = fqlPreproc.isFindByRowid();

+         boolean hasEnteredIteration = false;
+         
          while (true)
          {
             // Iterate all arguments provided for the base portion of the
@@ -5791,6 +5793,11 @@
                   }
                }

+               if (hasEnteredIteration)
+               {
+                  break;
+               }
+               
                // We are about to update the placeholder and try another pass,
                // so if the navigation request was for the first or last
                // record, we have to modify the mode to be next or previous,
@@ -5804,6 +5811,12 @@
                   case LAST:
                      activatePrevious();
                      break;
+                  case NEXT:
+                     hasEnteredIteration = true;
+                     break;
+                  case PREVIOUS:
+                     hasEnteredIteration = true;
+                     break;
                }

                // The found record must be skipped because it was either

#14 Updated by Tomasz Domin about 1 year ago

Eduard Soltan wrote:

Tomasz Domin wrote:

No wiki yet.
See #9270, #6860#80, support/harness/readme.txt (that will go into Wiki)
Attached zfile_set.txt for given tests to run.

But can I enter in the debugger in the culprit test class, would the issue still be reproduced?

Yes, I think so.

#15 Updated by Eduard Soltan about 1 year ago

Tomasz, could you run it again with 10083-13 patch?

#16 Updated by Tomasz Domin about 1 year ago

Eduard Soltan wrote:

Tomasz, could you run it again with 10083-13 patch?

I am working on this. Is this patch for 10083a or trunk ? I get rejects in both cases anyway for bzr patch and I had apply it manually. 

=== modified file 'src/com/goldencode/p2j/persist/RandomAccessQuery.java'
--- old/src/com/goldencode/p2j/persist/RandomAccessQuery.java    2025-06-03 05:45:13 +0000
+++ new/src/com/goldencode/p2j/persist/RandomAccessQuery.java    2025-06-06 06:33:03 +0000
@@ -5450,6 +5450,8 @@
          boolean needsLock = updateLock && lockType != LockType.NONE;
          boolean findByRowid = fqlPreproc.isFindByRowid();

+         boolean hasEnteredIteration = false;
+         
          while (true)
          {
             // Iterate all arguments provided for the base portion of the
@@ -5766,6 +5768,11 @@
                   }
                }

+               if (hasEnteredIteration)
+               {
+                  break;
+               }
+               
                // We are about to update the placeholder and try another pass,
                // so if the navigation request was for the first or last
                // record, we have to modify the mode to be next or previous,
@@ -5779,6 +5786,12 @@
                   case LAST:
                      activatePrevious();
                      break;
+                  case NEXT:
+                     hasEnteredIteration = true;
+                     break;
+                  case PREVIOUS:
+                     hasEnteredIteration = true;
+                     break;
                }

                // The found record must be skipped because it was either

#17 Updated by Tomasz Domin about 1 year ago

Eduard Soltan wrote:

Tomasz, could you run it again with 10083-13 patch?

The result stays the same:

1. test19LeakOnFindByRecordid: Test failure:org.opentest4j.AssertionFailedError:Expected: pi-2|si1-2|si2-2|uf-2 but was: ?
2. test20LeakOnRelease: Test failure:org.opentest4j.AssertionFailedError:Expected: pi-3|si1-3|si2-1|uf-6 but was: ?
3. test15LeakPreviousOnSecondaryIndexUpdate: Test failure:org.opentest4j.AssertionFailedError:Expected: pi-2|si1-2|si2-2|uf-2 but was: ?
4. test17NoLeadOnUnindexedFieldUpdate: Test failure:org.opentest4j.AssertionFailedError:Expected: pi-2|si1-2|si2-2|uf-2 but was: ?
5. test18LeakOnFindNotEquals: Test failure:org.opentest4j.AssertionFailedError:Expected: pi-2|si1-2|si2-2|uf-2 but was: ?
6. test16LeakCurrentOnFindNotEqual: Test failure:org.opentest4j.AssertionFailedError:Expected: pi-2|si1-2|si2-2|uf-2 but was: ?

#18 Updated by Eduard Soltan about 1 year ago

Tomasz Domin wrote:

Eduard Soltan wrote:

Tomasz, could you run it again with 10083-13 patch?

The result stays the same:
[...]

Are you sure that the patch was applied correctly?
I have run the TestUniqueDirtyShareSession1 and TestUniqueDirtyShareSession2 tests in debug mode in parallel with 10080a. rev. 15960 (#10083-1 patch) and indeed I get 5 tests more failing.

Running it with 10080a. rev. 15961 (#10083-13 patch) I do not get any failures. Except test20LeakOnRelease test.

#19 Updated by Tomasz Domin about 1 year ago

Eduard Soltan wrote:

Tomasz Domin wrote:

Eduard Soltan wrote:

Tomasz, could you run it again with 10083-13 patch?

The result stays the same:
[...]

Are you sure that the patch was applied correctly?
I have run the TestUniqueDirtyShareSession1 and TestUniqueDirtyShareSession2 tests in debug mode in parallel with 10080a. rev. 15960 (#10083-1 patch) and indeed I get 5 tests more failing.

Well, I wasnt sure thats why I asked what the patch is for :) as it didnt apply cleanly to 10083a/15960

Running it with 10080a. rev. 15961 (#10083-13 patch) I do not get any failures. Except test20LeakOnRelease test.

Do you mean 10083a/15961 as I can confirm that, results are fine with 10083a/15961.
Why #10080 is mentioned in 10083a/15961 ?

Code review: 10083a/15961
Please update RandomAccessQuery.java change log, otherwise I am fine with code changes.

I guess the change needs to go through extensive testing (regression + unit).
I am not sure how RAQ would behave in case there are two records in dirty share to be found by the query, not sure if support/harness/test/cross_session_test.xml covers the case.

#20 Updated by Eduard Soltan about 1 year ago

  • Status changed from Review to Internal Test

Tomasz Domin wrote:

Do you mean 10083a/15961 as I can confirm that, results are fine with 10083a/15961.
Why #10080 is mentioned in 10083a/15961 ?

Sorry, I have another 10080 task, and my thoughts were a bit mixed up. But yes I was talking about 10083a/15961.

Code review: 10083a/15961
Please update RandomAccessQuery.java change log, otherwise I am fine with code changes.

Added history entry, in rev. 15962.

#21 Updated by Eduard Soltan about 1 year ago

CHUI Regression tests passed with 10083a. ☑️
Large GUI Application unit tests passed. ☑️

#22 Updated by Eduard Soltan about 1 year ago

Rebased to latests trunk, and performed the following tests:

- smoke tests of a GUI application ☑️
- smoke and unit tests of a large GUI application ☑️
- chui regression testing ☑️
- fwd and unit tests ☑️
- harness/web/reports ☑️

I am not sure how RAQ would behave in case there are two records in dirty share to be found by the query, not sure if support/harness/test/cross_session_test.xml covers the case.

I think that is done in TestNonUniqueDirtyShareSession2.getTestRecords class method, it performs find first dsNonunique and then 2 times find next dsNonunique, and there 2 records in the dirty shared database.

#23 Updated by Constantin Asofiei about 1 year ago

I'll run ETF with this. Eduard, what's the state of the #10083-17 tests?

#24 Updated by Eduard Soltan about 1 year ago

Constantin Asofiei wrote:

I'll run ETF with this. Eduard, what's the state of the #10083-17 tests?

Passed with the latest revision of 10083a.

#25 Updated by Constantin Asofiei about 1 year ago

  • Assignee set to Eduard Soltan

ETF testing passed.

Is there anything else to test?

#26 Updated by Eduard Soltan about 1 year ago

Constantin Asofiei wrote:

ETF testing passed.

Is there anything else to test?

Stefanel tested another customer application and it went successfully.

#27 Updated by Constantin Asofiei about 1 year ago

  • Status changed from Internal Test to Merge Pending

Please merge now.

#28 Updated by Eduard Soltan about 1 year ago

  • Status changed from Merge Pending to Internal Test

10083a was merged to trunk rev 16023 and archived.

#29 Updated by Constantin Asofiei about 1 year ago

  • Status changed from Internal Test to Test

#30 Updated by Teodor Gorghe 11 months ago

  • Related to Bug #10390: FIND FIRST buffer field change does not propagate into the next FIRST FIRST statements. added

Also available in: Atom PDF