FWD » Core Development » Database

For a very long-running FWD context, there is an accumulation of TemporaryBuffer$Multiplexer instances in the finalizables of TransactionManager.globalBlock.

This may be just that the multiplexer instance needs to be cleaned up from the global finalizables, when the temp-table gets deleted.

Eric: do you recall if the global parameter at the TemporaryBuffer.define is used for global shared temp-tables, too?

Constantin Asofiei wrote:

Eric: do you recall if the global parameter at the TemporaryBuffer.define is used for global shared temp-tables, too?

Yes, AFAIK it is.

The recreate is this:

def temp-table tt1 field f1 as int.
def dataset ds1 for tt1.

def var ht as handle.
def var i as int.

create temp-table ht.
ht:create-like("tt1").
ht:temp-table-prepare("htt").

procedure proc0.
   def buffer btt1 for tt1.

   find first btt1 no-error.
end.

procedure proc1.
   def input parameter table for tt1.

   run proc0.
end.

do i = 1 to 1000000:
   run proc1(input table-handle ht by-reference).
end.

delete object ht.
message valid-handle(ht).

The conditions for the leak to occur are:

• have a buffer defined in an internal procedure
• the temp-table targeted by the explicit buffer definition is already bound to a dynamic temp-table
• when the explicit buffer's scope is opened in the internal procedure, this code in TemporaryBuffer.openScope():
  

           if (global)
           {
              // a global temp table buffer is cleaned up when the context ends
              txHelper.registerFinalizable(multiplexer, true);
  
              if (isDynamic())
              {
                 dynamicMultiplexer = multiplexer;
              }
           }
  

  • global set to true, because this is inherited from the 'master buffer' (the dynamic temp-table currently bound to tt1 buffer). This will registered the multiplexer in the TransactionManager$WorkArea.globalBlock.finalizables.
  • isDynamic() will be false, so dynamicMultiplexer will be null and this will never get de-registered.

The fix is this:

### Eclipse Workspace Patch 1.0
#P p2j6129a
Index: src/com/goldencode/p2j/persist/TemporaryBuffer.java
===================================================================
--- workspace.java.open.client/p2j6129a/src/com/goldencode/p2j/persist/TemporaryBuffer.java    (revision 3765)
+++ workspace.java.open.client/p2j6129a/src/com/goldencode/p2j/persist/TemporaryBuffer.java    (working copy)
@@ -743,7 +743,7 @@
    private static final Set<String> forceNoUndoConflicts = new HashSet<>();

    /** Does this buffer represent a global temp table? */
-   private final boolean global;
+   private boolean global;

    /** Can property changes on this buffer be undone? */
    private UndoStateProvider undoStateProvider;
@@ -1713,6 +1728,12 @@
                                             false,
                                             false);
             boundMaster.explicitBuffers.remove(newBuf);
+            
+            if (boundMaster.isDynamic())
+            {
+               // reset the 'global' flag for the newBuf
+               ((TemporaryBuffer) ((BufferReference) newBuf).buffer()).global = false;
+            }
          }
          finally
          {

The root cause was that when the explicit buffer was creating the binding to the master buffer's binding, the 'global' flag remained set, even if this new bounded buffer must not be considered as 'dynamic'.

Actually, the previous patch is incorrect. I think this code in TemporaryBuffer.define at line 1668 (branch 6129a):

      TemporaryBuffer buffer = makeBuffer(def,
                                          dmoBufIface,
                                          variable,
                                          masterBuf.global,
                                          masterBuf.undoStateProvider,
                                          blockDepth);

should always pass false instead of masterBuf.global, as this define API as far as I can see is used only internally in FWD.

Why I say this: the previous fix doesn't solve all issues, the same problem exists if a dataset parameter exists in the same procedure which has the DEFINE BUFFER.

Constantin Asofiei wrote:

Actually, the previous patch is incorrect. I think this code in TemporaryBuffer.define at line 1668 (branch 6129a):
[...]
should always pass false instead of masterBuf.global, as this define API as far as I can see is used only internally in FWD.

I'm not sure what you mean by "is used only internally in FWD". There are many call paths which lead to this define method variant, including from other, public define variants, some of which are called by SharedVariableManager methods.

Why I say this: the previous fix doesn't solve all issues, the same problem exists if a dataset parameter exists in the same procedure which has the DEFINE BUFFER.

This looks a little suspect (at line 798):

   /**
    * For a master buffer, all explicit buffers created for it.
    * TODO: I think there is a memory leak hear, needs to be reviewed.
    */
   private final Set<Temporary> explicitBuffers = Collections.newSetFromMap(new IdentityHashMap<>());

I think I have a better fix for this leak; in TemporaryBuffer.initialize do this:

      if (global && master != null && master.isDynamic() && !isDynamic())
      {
         global = false;
      }

This solves the Multiplexer leak in globalBlock.finalizables, but looking at the heap there is an accumulation of dynamic QueryWrapper instances in the handle resources - some are invalid (deleted), some are still valid. I'm looking into this.

Constantin Asofiei wrote:

I think I have a better fix for this leak; in TemporaryBuffer.initialize do this:
[...]

global initially is set in the TemporaryBuffer constructor. initialize can be called at any time, from many different locations (though only the first call actually does the work; all subsequent calls return immediately). Considering this variability in when initialize is called, is overriding global in initialize reliably early enough to prevent the leak?

Eric Faulhaber wrote:

Constantin Asofiei wrote:

I think I have a better fix for this leak; in TemporaryBuffer.initialize do this:
[...]

global initially is set in the TemporaryBuffer constructor. initialize can be called at any time, from many different locations (though only the first call actually does the work; all subsequent calls return immediately). Considering this variability in when initialize is called, is overriding global in initialize reliably early enough to prevent the leak?

Yes, because the Multiplexer is registered in initialize, after the global is overriden to false.