FWD » Core Development » Base Language

TransactionManager.processFinallyBlock is not protected at all for conditions raised from within the finally block. Any condition will abend the processing of:

iterateWorker(WorkArea)
popScope(WorkArea)
processRetry(WorkArea, BlockDefinition)

and leaves the context in a corrupted state.

I saw this in a scenario where there an ERROR condition 12327, Cannot delete a BY-REFERENCE PARAMETER dataset or table in the called procedure is raised from within the finally block, which 'abends' the popScope processing (as a side note, I need to investigate this - if it should be raised or not - but regardless this does not affect this finally abend problem in FWD).

My first instinct would be to execute the Block.fini call from a BlockManager.doBlock (so all block processing is in place).

I need to write some testcases and see if some conditions are 'consumed' and others are honored from the finally block.

My first problem is that I don't see how to distinguish between undo, leave and leave, when is caught via LeaveUnwindException. I think maybe these statements are no-op if they are executed in the finally, and they target the owner block.

We could create UndoLeaveUnwindException which is a subclass of LeaveUnwindException. Normal cases would continue to work, but this special case could be detected.

I think depends if the owner block has an ON clause or not.

For DO blocks, only DO TRANSACTION and DO ON ERROR support FINALLY. I don't think DO ON ENDKEY will work, for example.

Greg, a change I think is most likely needed is to move everything after the processFinallyBlock in a finally, so that is executed regardless if there is a condition thrown or not.

Agreed. Please put a new try/finally in like this from processRetry():

...
      try
      {
         // my excellent comments :)
         processFinallyBlock(wa, blk);
      }

      finally
      {
         wa.handleDeferredError();

         // check the interrupted status and throw the stop condition if needed
         wa.honorStopCondition();
      }

I don't want to move the following logic into an existing finally block. The reason: this would change the behavior in the case where something is raised inside that following logic. Things like handleDeferredError() are where they are for a reason.

Make sure to put your testcases in testcases/uast/finally/ and update the readme there.

Greg Shah wrote:

Agreed. Please put a new try/finally in like this from processRetry():

[...]

I don't want to move the following logic into an existing finally block. The reason: this would change the behavior in the case where something is raised inside that following logic. Things like handleDeferredError() are where they are for a reason.

Yes, that was my intent for moving the code.

OTOH, just this change stabilizes FWD and the original abend in the customer application is solved.

I suggest leaving this on the 'back burner' while I work on some more high priority tasks.

I think you can go ahead with the change.

I just found that another customer application is using RETURN "<val>" from the finally block.

I'll right a report to find which kind of tokens are used from a finally block, and fix those at this time.

I think we need to handle all the cases now. We can't know what minor edit will cause new cases to be used tomorrow which are not there today. I don't want to have to debug this later to find out that something is missing.

Greg, there is an issue which is kind of tricky. For looping blocks, the finally can not be processed at the popScope() - this is because for example at the finally you can have a NEXT and at the block you can have a LEAVE - the NEXT in the FINALLY will override the LEAVE:

def var k as int.
def var i as int.

i = 0.
k = 0.
repeat i = 1 to 100  :
  k = k + 1. 
  leave.
  finally:
     k = k + 1.
     next .
  end.
end.
message k i.

If I let the finally block to be executed in popScope (after the LEAVE), then there is no way for the loop to resume processing (and for FINALLY to override conditions/unwind from the main block).

For this reason I need to move the blockSetup inside the inner 'do ... while' loop for i.e. BlockManager.coreLoop:

         loop:     
         while (true)
         {
            boolean runBlockSetup = true;

            do
            {
               try
               {
                  if (runBlockSetup)
                  {
                     wa.tm.blockSetup();
                     runBlockSetup = false;
                  }
                  ...
                  if (!needsIterate(wa.tm, to, expr))
                  {
                     break loop;
                  }

                  processBody(wa, block, on);
               }
               ... // all the other catch blocks
               catch (Throwable thr)
               {
                  wa.tm.abnormalEnd(thr);
               }
            }
            while (wa.tm.needsRetry());

            if (wa.tm.isBreakPending())
            {
               break loop;
            }
         }

needsIterate had to be moved after blockSetup, so that it will be executed after the FINALLY block (and not before it), for an iteration.

My problem is I don't see how safe is that the tx-related logic is executed twice at the end of the loop, once for the blockSetup (for the last FINALLY call) and once in popScope.

As a side note, I need to duplicate my tests with other block types (currently I used only REPEAT, and had to refactor coreLoop a lot...).

Ughhh. Supporting NEXT in a finally which overrides a prior LEAVE is an awful "feature" and a terrible idea. What could they have been thinking when they implemented that? Or perhaps they didn't think about it at all and it is just one more poorly conceived implementation quirk.

I'm still thinking about this, but I don't think it is right to use blockSetup() for the popScope() execution of finally. It will break the transaction processing at a minimum. The other changes to make it work also are quite dangerous. For example, needsIterate() cannot be executed inside the needsRetry() inner loop. The retry is independently processed and needsIterate() must never re-execute (there can be side effects from the WHILE expression). Even if we avoid the re-execution, I think there are problems with the ordering. And these changes also seem likely to break the control flow for how we process exceptions.

There must be a better way. I'll think about this some more.

And something I didn't expect: same suite ran in ChUI and GUI (OE 11.6.3) gives different results in ~5% of cases. They all seem to be part of some cases where a raised ERROR condition is involved.

I am willing to postpone the implementation of NEXT in a FINALLY. We can put code in to detect this at conversion and render a WARNING.

Are there other cases that would drive a radical rework of our block processing? If not, let's create a separate task and work it later.

Greg, to answer your question, yes, the unwind can be overridden in the FINALLY (via LEAVE/NEXT/RETRY or even another condition). The problem is only the owner block (of the finally) decides to unwind, and the FINALLY block overrides this and decides to continue the loop (as in this case FINALLY will be executed from popScope).

Leaving the 'unwind override' from the FINALLY aside, the solution was to catch any unwind/condition from the FINALLY, deffer it and throw it in processBody - so it will be treat it in the main code for this block.

But, the FINALLY block needs to be executed before the loop expression (which can be whatever, a WHILE <expr> or var to expr()). So I need the coreLoop to look like this:

         boolean firstRun = true;
         boolean secondRun = false;

         loop:     
         while (true)
         {
            if (firstRun)
            {
               if (!needsIterate(wa.tm, to, expr))
               {
                  break loop;
               }

               wa.tm.blockSetup();
               firstRun = false;
            }

            if (secondRun)
            {
               wa.tm.blockSetup();

               if (wa.deferredFinallyThrow == null && !needsIterate(wa.tm, to, expr))
               {
                  break loop;
               }
            }
            secondRun = true;

But I think you are right, this change still seems dangerous, as the blockSetup will be executed one additional time (for iteration mode) than the current way. I'll comment this firstrun/secondRun code and leave it behind, to at least have some 'breadcrumbs' for when we get back to it.

I've tested with 'before and after' and there are 59 failures which should be related to 'unwind override' and additional 67 failures if I comment out the firstRun/secondRun code (this is from a 819 test suite).

3821c/13001 adds the try/finally protection for TM.processFinallyBlock.

Code Review Task Branch 3821c Revision 13001

No objections.

Are there any known customer issues that are caused by the remaining issues? If not, we will defer those items.

Greg Shah wrote:

Are there any known customer issues that are caused by the remaining issues? If not, we will defer those items.

Yes, there are QUIT, STOP, RETURN and some other cases where an ERROR condition can be thrown. I need to stabilize these changes and finish the FINALLY for the top-level block.

Changes on #4602 where designed in a general way to handle catch and finally blocks. So I think they could handle a big chunk of the finally block issues. Right now I will focus on refactoring tests written for catch blocks.

Constantin Asofiei wrote:

Greg, there is an issue which is kind of tricky. For looping blocks, the finally can not be processed at the popScope() - this is because for example at the finally you can have a NEXT and at the block you can have a LEAVE - the NEXT in the FINALLY will override the LEAVE:

After running some generated testes and solving some minor issues, I think I stumble across the same problem exposed here.

for each tt2 on error undo, leave:
   k = k + 1.
   h = h:next-sibling.

   finally :              
      k = k + 1.
      next.
   end.
end.

h = h:next-sibling. raised the error condition which is handled by FWD with a LEAVE of for-each block according to on error undo, leave directive. I have some implementation in FWD (4602a) for catch/finally blocks to be executed after the block iteration (not in blockSetup or popScope).

However the problem is that the error handling logic in forEachWorker will make the decision on next step in the block flow even before the finally is executed (CONTINUE/BREAK on the forEachWorker). finally block is executed but it doesn't have any say in subsequent block flow, even if an condition is raised.

I think solution would be to have a pending control flow action registered in block error handling logic, which could be overwritten finally block.

+ watchers

I have stumbled across a different issue.

block-level on error undo, throw.
def var h as handle.
def var k as int.
def var i as int.

k = 1.
i = 1.

procedure func1:
   k = k + 1.
   h = h:next-sibling.

   finally:
      message "First Finally".
      k = k + 1.
      STOP.
   end.
end.

doBlock1:
do i = 1 to 3 on error undo, leave
              on stop undo, leave
              on quit undo, next:
   message i.
   run func1.

   message "After procedure call".

   finally:
      message "Second Finally".
      k = k + 1.
   end.
end.

message "Left Do Block".

This test behaves weirdly in OE 11.6.3.
Error in func1 procedure is caused by reference on uninitialized handle h. In finally block another control-of-flow statement is called and and it seems to override the error raised in the main block. However directive (on stop undo, leave) in doBlock1 is not respected, and it leaves the procedure entirely.

If I use on stop undo, next directive instead, it will iterate to the last iteration and again will leave the whole procedure entirely.

This test performed on OE 11.7 onward, behaves as expected. This means that stop in finally block will override the error raised previously, and the directives instruction will be respected strictly.

Investigation upon condition raised power¶

I noticed that the override of the error or control-of-flow statement from the finally does not necessarily happens in all cases.

block-level on error undo, throw. 

procedure func1:
   k = k + 1.
   h = h:next-sibling.

   finally:
      message "FIRST Finally".
      k = k + 1.
      STOP.
   end.
end.

doBlock1:
do i = 1 to 3 on error undo, next
              on stop undo, leave
              on quit undo, next:
   message i.
   run func1.

   message "after".

   catch e as Progress.Lang.Error:    
      message "ERROR" e:GetMessage(1).
   end catch.
end.

Because of the presence of catch in the the caller block, the h = h:next-sibiling() error is raised as Progress.Lang.Error exception. Now in this case the simple stop condition raised from finally does not have any effect on condition raised previously. For this case I consider that Progress.Lang.Error is more powerful that the simple stop or error condition.

If however I add in doBlock1 a catch block for Progress.Lang.Stop and use -catchStop 1 startup parameter, of course:

catch e as Progress.Lang.Stop:    
   message "Stop".
end catch.

For this case stop condition raised as Progress.Lang.Stop will override the Progress.Lang.Error raised previously. So this proves that Progress.Lang.Error and Progress.Lang.Stop has equal power.

block-level on error undo, throw.
def var h as handle.
def var k as int.
def var i as int.

def temp-table tt2 field f1 as int.

create tt2. tt2.f1 = 1. release tt2.
create tt2. tt2.f1 = 2. release tt2.
create tt2. tt2.f1 = 3. release tt2.
create tt2. tt2.f1 = 4. release tt2.
create tt2. tt2.f1 = 5. release tt2.

k = 1.
i = 1.

procedure func1:

   forBlock:
   for each tt2:
      k = k + 1.
      STATEMENT1_PLACEHOLDER.

      finally:
         k = k + 1.

         STATEMENT2_PLACEHOLDER.
      end.
   end.
end.

def var g as int.

do i = 1 to 3 on error undo, leave
              on stop undo, next
              on quit undo, next:

   run func1.

   STOP_CATCH_PLACEHOLDER.                         

   ERROR_CATCH_PLACEHOLDER.
end.

message "LEFT DO BLOCK1".

In the above pseudocode I have 4 placeholders: STATEMENT1_PLACEHOLDER (used to be replaced with different control of flow statements in FOREACH body), STATEMENT2_PLACEHOLDER (used to be replaced with different control of flow statements in FINALLY body), STOP_CATCH_PLACEHOLDER (for present or absence of CATCH stop block) and STATEMENT2_PLACEHOLDER (for present or absence of CATCH error block).

In the table I recorded my empirical results of the behaviour of the program with different statement combinations.

Results¶

As a result of my experiments I could conclude that different condition have different strengths:

control-of-flow (leave, next, retry) < error/stop < Progress.Lang.Error/Progress.Lang.Stop

And condition raised from finally will override the condition from main block only if strength of condition raised in FINALLY >= strength of condition raised in MAIN BLOCK

Very good work!

Another thing to notices, even the error thrown from finally block of the finally should be overridden by the same rule.

finally:
      message "First Finally".
      k = k + 1.

      h = h:next-sibling.
      finally:
         STOP.
      end.
   end.

I have made the implementation for the override of the error thrown from main block, by the error thrown from the finally block. The changes however however very much depends from some other changes from 4602a, should I keep committing to 4602a or create another branch for that?

Eduard Soltan wrote:

Another thing to notices, even the error thrown from finally block of the finally should be overridden by the same rule.

[...]

I have made the implementation for the override of the error thrown from main block, by the error thrown from the finally block. The changes however however very much depends from some other changes from 4602a, should I keep committing to 4602a or create another branch for that?

Create a branch from 4602a and will rebase it from trunk once 4602a is in trunk (hopefully this week).