Wicket 9.0.0 AsynchronousPageStore Potential Memory Leak

[Todd Heaps]

In Wicket 8.3.0, I had to override AsynchronousPageStore to fix a memory
leak. In looking at older versions of 9.0.0's version
of AsynchronousPageStore , I thought the memory leak had been fixed, but
now that 9.0.0 has been released (and maybe it was always that way and I
wasn't seeing it right), it seems like the memory leak is still there.

Here is 8.3.0's AsynchronousPageStore.PageSavingRunnable inner class with
the memory leak:

 private class PageSavingRunnable implements Runnable
 {
  @Override
  public void run()
  {
   while (pageSavingThread != null)
   {
    Entry entry = null;
    try
    {
     entry = entries.poll(POLL_WAIT, TimeUnit.MILLISECONDS);
    }
    catch (InterruptedException e)
    {
     log.debug("PageSavingRunnable:: Interrupted...");
    }
    if (entry != null && pageSavingThread != null)
    {
     log.debug("PageSavingRunnable:: Saving asynchronously: {}...", entry);
     delegate.storePage(entry.sessionId, entry.page);
     entryMap.remove(getKey(entry));
    }
   }
  }
 }

Note that in the code above if an exception is thrown during the call:
delegate.storePage(entry.sessionId, entry.page)

the call:
entryMap.remove(getKey(entry))

never happens, causing the entryMap to continue to gradually increase in
size, eventually causing the heap to run out of memory.

I switched those two lines to be:
     entryMap.remove(getKey(entry));
     delegate.storePage(entry.sessionId, entry.page);

Now if an exception is thrown in delegate.storePage(), there is no longer a
problem because the page entry has already been removed.  This has been
confirmed in our code base where after making the change, we no longer had
memory leak problems.

Now in Wicket 9.0.0 very similar code looks like it will have the same
problem. Note the following in AsynchronousPageStore.PageAddingRunnable:

 private static class PageAddingRunnable implements Runnable
 {
  private static final Logger log =
LoggerFactory.getLogger(PageAddingRunnable.class);
  private final BlockingQueue<PendingAdd> queue;
  private final ConcurrentMap<String, PendingAdd> map;
  private final IPageStore delegate;
  private PageAddingRunnable(IPageStore delegate, BlockingQueue<PendingAdd>
queue,
                             ConcurrentMap<String, PendingAdd> map)
  {
   this.delegate = delegate;
   this.queue = queue;
   this.map = map;
  }
  @Override
  public void run()
  {
   while (!Thread.interrupted())
   {
    PendingAdd add = null;
    try
    {
     add = queue.poll(POLL_WAIT, TimeUnit.MILLISECONDS);
    }
    catch (InterruptedException e)
    {
     Thread.currentThread().interrupt();
    }
    if (add != null)
    {
     log.debug("Saving asynchronously: {}...", add);
     add.asynchronous = true;
     delegate.addPage(add, add.page);
     map.remove(add.getKey());
    }
   }
  }
 }

If an exception is thrown during the call:
delegate.addPage(add, add.page);

The call to:
map.remove(add.getKey());

never happens, which will presumably still cause a memory leak.

We're in the process of migrating from Wicket 8.x to 9.0.0 and will be
running 9.0.0 in a production environment soon.  If needed, we'll try to
implement a similar fix for this in 9.0.0,

Do you think this is something that would be important enough to be looked
into soon?

Thanks for your time, and thanks for Wicket.  It's a great web framework.
We've been using it for many years now.

Todd Heaps