You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@wicket.apache.org by Todd Heaps <he...@gmail.com> on 2020/07/22 10:53:08 UTC
Wicket 9.0.0 AsynchronousPageStore Potential Memory Leak
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
Re: [EXTERNAL] Re: Wicket 9.0.0 AsynchronousPageStore Potential
Memory Leak
Posted by Todd Heaps <to...@westringtechnologies.com>.
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 <mg...@apache.org>
Sent: Friday, August 28, 2020 5:40 AM
To: dev@wicket.apache.org <de...@wicket.apache.org>
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 <mg...@apache.org>
wrote:
> Hi,
>
> On Wed, Jul 22, 2020 at 1:53 PM Todd Heaps <he...@gmail.com> 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
>>
>
Re: Wicket 9.0.0 AsynchronousPageStore Potential Memory Leak
Posted by Martin Grigorov <mg...@apache.org>.
https://issues.apache.org/jira/browse/WICKET-6822
On Wed, Jul 22, 2020 at 3:03 PM Martin Grigorov <mg...@apache.org>
wrote:
> Hi,
>
> On Wed, Jul 22, 2020 at 1:53 PM Todd Heaps <he...@gmail.com> 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
>>
>
Re: Wicket 9.0.0 AsynchronousPageStore Potential Memory Leak
Posted by Martin Grigorov <mg...@apache.org>.
Hi,
On Wed, Jul 22, 2020 at 1:53 PM Todd Heaps <he...@gmail.com> 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
>
Re: Wicket 9.0.0 AsynchronousPageStore Potential Memory Leak
Posted by Sven Meier <sv...@meiers.net>.
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
>