Uploaded image for project: 'Wicket'
  1. Wicket
  2. WICKET-6822

AsynchronousPageStore Potential Memory Leak

Attach filesAttach ScreenshotVotersWatch issueWatchersCreate sub-taskLinkCloneUpdate Comment AuthorReplace String in CommentUpdate Comment VisibilityDelete Comments
    XMLWordPrintableJSON

Details

    • Bug
    • Status: Resolved
    • Major
    • Resolution: Fixed
    • 9.0.0, 8.9.0
    • 9.1.0, 8.10.0
    • wicket
    • None

    Description

      From the dev@ mailing list:

       

      ------------------------------------------------

      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

      Attachments

        Activity

          This comment will be Viewable by All Users Viewable by All Users
          Cancel

          People

            mgrigorov Martin Tzvetanov Grigorov
            mgrigorov Martin Tzvetanov Grigorov
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Slack

                Issue deployment