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 <mgrigo...@apache.org> Sent: Friday, August 28, 2020 5:40 AM To: dev@wicket.apache.org <dev@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 <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 >> >
