Hi,
On Wed, Jul 22, 2020 at 1:53 PM Todd Heaps <[email protected]> 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
>
