https://issues.apache.org/jira/browse/WICKET-6822
On Wed, Jul 22, 2020 at 3:03 PM Martin Grigorov <mgrigo...@apache.org>
wrote:
> Hi,
>
> On Wed, Jul 22, 2020 at 1:53 PM Todd Heaps <heaps...@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
>>
>
