Uploaded image for project: 'Pax Logging'
  1. Pax Logging
  2. PAXLOGGING-27

IndexOutOfBoundsException in EventAdminTracker

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.0-RC2
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None

      Description

      Occasionally, we get the following exception:

      java.lang.IndexOutOfBoundsException: Index: 0, Size: 0
      at java.util.LinkedList.entry(LinkedList.java:365)
      at java.util.LinkedList.remove(LinkedList.java:357)
      at org.ops4j.pax.logging.EventAdminTracker.deliver(EventAdminTracker.java:94)
      at org.ops4j.pax.logging.EventAdminTracker.postEvent(EventAdminTracker.java:56)

      The fix is trivial, i think: in the EventAdminTracker.deliver, a (synchronized!) check on the queue size is missing:

      while( m_queue.size() > 0 )
      {
      Event event;
      synchronized( m_queue )

      { event = (Event) m_queue.remove( 0 ); }

      }

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            niclas@hedhman.org Niclas Hedhman added a comment -

            Feel free to commit yourself...

            Show
            niclas@hedhman.org Niclas Hedhman added a comment - Feel free to commit yourself...
            Hide
            peter.doornbosch@gmail.com Peter Doornbosch added a comment -

            ok

            Show
            peter.doornbosch@gmail.com Peter Doornbosch added a comment - ok
            Hide
            peter.doornbosch@gmail.com Peter Doornbosch added a comment -

            So, i fixed the exception by adding an if statement within the synchronized. Although this fixes the bug (i.e. the IndexOutOfBoundsException), this does not guarantee that all events are processed, because the test in the while condition is not synchronized and the member is not volatile. Hence, an update on an other thread might not be seen...
            To solve this, it would be better to rewrite the while loop, i guess; something like:
            do {
            sync'd( m_queue )

            { if (m_queue.size() > 0) event = ... }

            if (event != null)
            ....
            }
            while (event != null)

            Btw. the m_service EventAdmin member also suffers from not being volatile, while being accessed by multiple threads without proper synchronization.

            Show
            peter.doornbosch@gmail.com Peter Doornbosch added a comment - So, i fixed the exception by adding an if statement within the synchronized. Although this fixes the bug (i.e. the IndexOutOfBoundsException), this does not guarantee that all events are processed, because the test in the while condition is not synchronized and the member is not volatile. Hence, an update on an other thread might not be seen... To solve this, it would be better to rewrite the while loop, i guess; something like: do { sync'd( m_queue ) { if (m_queue.size() > 0) event = ... } if (event != null) .... } while (event != null) Btw. the m_service EventAdmin member also suffers from not being volatile, while being accessed by multiple threads without proper synchronization.
            Hide
            niclas@hedhman.org Niclas Hedhman added a comment -

            Due to this and other deadlock issues, I wonder if we for 1.2 should just introduce a FIFO on the incoming log requests which are dispatched to a thread owned by Pax Logging itself.

            That would make the synchronization needed in Pax Logging extremely well contained and easily understood, as well as solving this and the deadlock with Felix internals, and we can guarantee that we don't hold on to any locks in the thread calling out to Event Admin, while triggering classloading and so forth.

            Show
            niclas@hedhman.org Niclas Hedhman added a comment - Due to this and other deadlock issues, I wonder if we for 1.2 should just introduce a FIFO on the incoming log requests which are dispatched to a thread owned by Pax Logging itself. That would make the synchronization needed in Pax Logging extremely well contained and easily understood, as well as solving this and the deadlock with Felix internals, and we can guarantee that we don't hold on to any locks in the thread calling out to Event Admin, while triggering classloading and so forth.
            Hide
            karlpauls Karl Pauls added a comment -

            For what it's worth, the deadlock issue with Felix internals should be fixed as of Felix 1.0.4.

            Show
            karlpauls Karl Pauls added a comment - For what it's worth, the deadlock issue with Felix internals should be fixed as of Felix 1.0.4.
            Hide
            niclas@hedhman.org Niclas Hedhman added a comment -

            This has been fixed by PeterD himself in commit 11686. Will be part of 1.2 release, or sooner.

            Show
            niclas@hedhman.org Niclas Hedhman added a comment - This has been fixed by PeterD himself in commit 11686. Will be part of 1.2 release, or sooner.

              People

              • Assignee:
                niclas@hedhman.org Niclas Hedhman
                Reporter:
                peter.doornbosch@gmail.com Peter Doornbosch
              • Votes:
                0 Vote for this issue
                Watchers:
                0 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Development