FWD » Core Development » Base Language

In #8308, it was discovered that errors related to the database are handled differently when a CATCH block is present in the current block. These changes reached trunk in trunk/rev.15255 and 7156b/rev.15076. #8308-212 - #8308-237 are the notes that refer to this issue. In #8308-222 and #8308-225, two lists of places where recordOrShowError() is using isError = false.

TBD: There are cases that were not tested, check the two lists provided and investigate them (it is important to look at the persist package), write tests for each one and fix them using recordOrShowDatabaseError() if necessary. (reference tests: #8308-212, #8308-219, #8308-220, #8308-235).

Created 8643a.

Committed 8643a/rev.15307 which contains the 15272 and 15273 revisions from 8308e. (see #8308-397)

Dănuț Filimon wrote:

Committed 8643a/rev.15307 which contains the 15272 and 15273 revisions from 8308e. (see #8308-397)

Those will need to be discarded completely .

Constantin Asofiei wrote:

Dănuț Filimon wrote:

Committed 8643a/rev.15307 which contains the 15272 and 15273 revisions from 8308e. (see #8308-397)

Those will need to be discarded completely .

I reverted the changes from revision 8643a/rev.15307 in 8643a/rev.15308.

I've rebased 7643a from trunk rev 15308.

The changes are in rev 15311. With this, the tests in #8308-397, #8308-306 and #8308-319 are working.

Danut: please run the tests for #8308 and post any details at that task (not in this task).

Constantin, patching ErrorManager for 7156b is a bit hard to understand as there are no ServerErrorDataAccessor and SuppressedErrorDataAccessor in 7156b, only LocalDataAccess, ServerDataAccess and ErrorDataAccess.

From what I can understand, ErrorDataAccess is actually ServerErrorDataAccessor and SuppressedErrorDataAccessor is actually ServerDataAccess but it uses false as a return value by default while in 7156b it uses ErrorManager. I've managed to patch the other two files, but this one is hard to understand as there is no javadoc that explains these classes in detail.

I've posted my results in #8308-403 where I compared the previously made changes from 8308e with the new changes from 8643a. From the note:

Previously there were 26 tests with 18 fixed by 8308e, 3 displaying a known error, 5 that had different errors. Now there are 17 fixed with 7 that have the index issue and 2 that still need to be fixed.

8643a changes fail this test where the error is shown into a modal instead of being caught by the catch block:

DEFINE TEMP-TABLE ttsrc13 BEFORE-TABLE bittsrc13 FIELD srcid AS INT FIELD srcname AS CHAR.
DEFINE TEMP-TABLE src13 FIELD srcid AS INTEGER FIELD srcname AS CHARACTER INDEX srcid IS UNIQUE srcid.
DEFINE DATASET dssrc13 FOR ttsrc13.

DEFINE VARIABLE httsrc AS HANDLE NO-UNDO.
DEFINE VARIABLE hsrc AS HANDLE NO-UNDO.

TEMP-TABLE ttsrc13:TRACKING-CHANGES = TRUE.
CREATE ttsrc13. ASSIGN ttsrc13.srcid = 1 ttsrc13.srcName = 's1'.
CREATE ttsrc13. ASSIGN ttsrc13.srcid = 1 ttsrc13.srcName = 's2'.
TEMP-TABLE ttsrc13:TRACKING-CHANGES = FALSE.

httsrc = BUFFER ttsrc13:HANDLE.
hsrc = BUFFER src13:HANDLE.

DEFINE VARIABLE srcsrc13 AS HANDLE NO-UNDO.
CREATE DATA-SOURCE srcsrc13.
srcsrc13:ADD-SOURCE-BUFFER(hsrc, "srcname,srcid").
httsrc:ATTACH-DATA-SOURCE(srcsrc13, "srcid,srcid,srcname,srcname").

DO TRANSACTION ON ERROR UNDO, THROW:
    FOR EACH bittsrc13 ON ERROR UNDO, THROW:
        BUFFER bittsrc13:SAVE-ROW-CHANGES().
    END.
    CATCH ex AS Progress.Lang.Error :
        MESSAGE 132 ex:GetMessageNum(1).
    END CATCH.
END.

It's caused by the revalidation of the record in BlockManager.processForBody():

         if (reason == null || isNormal(wa, reason, on))
         {
            executeStateManaged(wa, wa.tm::processValidate);
         }

I've mentioned this in an email but never in an issue so I'll be writing it here. There was a problem in #8308 with the introduction of the error 11910 when there was a validation exception in BufferImpl.saveRowChangesImpl2() and this was causing a "double validation" in TxWrapper$WorkArea.validate(). The fix is actually simple, if a record is in an INVALID state, it should not be revalidated as it already failed it once.

=== modified file 'src/com/goldencode/p2j/persist/RecordBuffer.java'
--- old/src/com/goldencode/p2j/persist/RecordBuffer.java    2024-06-26 06:05:25 +0000
+++ new/src/com/goldencode/p2j/persist/RecordBuffer.java    2024-06-26 09:44:02 +0000
@@ -6233,7 +6233,8 @@
       if (!isActive()                        ||
           currentRecord == null              ||
           !inChangeScope() && !isTransient() ||
-          currentRecord.checkState(DmoState.DELETING))
+          currentRecord.checkState(DmoState.DELETING) ||
+          currentRecord.checkState(DmoState.INVALID))
       {
          // nothing to do
          return;

The change mentioned in #8643-15 is not ok, the method called is RecordBuffer.validate() which calls flush(true) and the true value enables revalidating the record. It will require another fix.

Danut, trunk/14998 was especially created to handle rollbacks, as there were unit tests (on another customer application) that were not properly rollbacking on failure. You can cherry-pick these. But, you will also need trunk/15244 that is fixing a regression of 14998.

Alexandru Lungu wrote:

Danut, trunk/14998 was especially created to handle rollbacks, as there were unit tests (on another customer application) that were not properly rollbacking on failure. You can cherry-pick these. But, you will also need trunk/15244 that is fixing a regression of 14998.

Thanks! I'll take those changes and test them.

I've tried using BlockManager and TransactionManager from trunk (not just revs 14998/15244, so other changes, too), and this caused lots of regressions in the customer's unit tests. If you decide to include just the changes from 14998 and 15244, you need do a full run of both unittests and fwdtests.

Danut, please pend effort until I can get some logs from 6667g to cross-check.

Constantin Asofiei wrote:

I've tried using BlockManager and TransactionManager from trunk (not just revs 14998/15244, so other changes, too), and this caused lots of regressions in the customer's unit tests. If you decide to include just the changes from 14998 and 15244, you need do a full run of both unittests and fwdtests.

I only plan to test 14998 and 15244 with 8643a changes, can you take a look at #8643-12 and let me know if this patch is ok for 7156b?

Alexandru Lungu wrote:

Danut, please pend effort until I can get some logs from 6667g to cross-check.

This issue is related to error handling, is there something from 6667g that should be taken into account here?

6667g is based on trunk rev/15271, so it includes both revisions.

Dănuț Filimon wrote:

... can you take a look at #8643-12 and let me know if this patch is ok for 7156b?

The patch is OK (considering forceExplicit was added after).

Alexandru Lungu wrote:

6667g is based on trunk rev/15271, so it includes both revisions.

I understand now, I'll mark the tests solved by this issue as "fixed" so that I don't investigate them by mistake while continuing my work on the customer module.