Wicket 9.0.0 AsynchronousPageStore Potential Memory Leak

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

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
Reply | Threaded
Open this post in threaded view
|

Re: Wicket 9.0.0 AsynchronousPageStore Potential Memory Leak

Martin Grigorov-4
Hi,

On Wed, Jul 22, 2020 at 1:53 PM Todd Heaps <[hidden email]> wrote:

> 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?
>

Sure!
Just file a ticket at https://issues.apache.org/jira/browse/WICKET !
You could even send us a Pull Request at https://github.com/apache/wicket with
the change!
Thanks!


> 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
>
Reply | Threaded
Open this post in threaded view
|

Re: Wicket 9.0.0 AsynchronousPageStore Potential Memory Leak

Sven Meier
In reply to this post by Todd Heaps
Hi Todd,

has this problem been reported before? It seems that problem have been
lurking there forever.

What I'm wondering: If saving of any page to the delegate fails, all
further page saving can no longer be asynchronous, isn't it?
The runnable's thread will just stop and all following pages will be
stored synchronously.

Have fun
Sven


On 22.07.20 12:53, Todd Heaps wrote:

> 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
>
Reply | Threaded
Open this post in threaded view
|

Re: Wicket 9.0.0 AsynchronousPageStore Potential Memory Leak

Martin Grigorov-4
In reply to this post by Martin Grigorov-4
https://issues.apache.org/jira/browse/WICKET-6822

On Wed, Jul 22, 2020 at 3:03 PM Martin Grigorov <[hidden email]>
wrote:

> Hi,
>
> On Wed, Jul 22, 2020 at 1:53 PM Todd Heaps <[hidden email]> wrote:
>
>> 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?
>>
>
> Sure!
> Just file a ticket at https://issues.apache.org/jira/browse/WICKET !
> You could even send us a Pull Request at https://github.com/apache/wicket with
> the change!
> Thanks!
>
>
>> 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
>>
>
Reply | Threaded
Open this post in threaded view
|

Re: [EXTERNAL] Re: Wicket 9.0.0 AsynchronousPageStore Potential Memory Leak

Todd Heaps-2
Thanks so much for filing the ticket.  I'm sorry that I didn't get it done myself.  We have been really busy here.
________________________________
From: Martin Grigorov <[hidden email]>
Sent: Friday, August 28, 2020 5:40 AM
To: [hidden email] <[hidden email]>
Subject: [EXTERNAL] Re: Wicket 9.0.0 AsynchronousPageStore Potential Memory Leak

WARNING: This email originated from outside the organization. Do not click links, open attachments, or enter user credentials, unless you recognize the sender and know the content is safe.


https://issues.apache.org/jira/browse/WICKET-6822

On Wed, Jul 22, 2020 at 3:03 PM Martin Grigorov <[hidden email]>
wrote:

> Hi,
>
> On Wed, Jul 22, 2020 at 1:53 PM Todd Heaps <[hidden email]> wrote:
>
>> 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?
>>
>
> Sure!
> Just file a ticket at https://issues.apache.org/jira/browse/WICKET !
> You could even send us a Pull Request at https://github.com/apache/wicket with
> the change!
> Thanks!
>
>
>> 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
>>
>