Pax Logging
  1. Pax Logging
  2. PAXLOGGING-27

IndexOutOfBoundsException in EventAdminTracker

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major 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 ); }

      }

        Activity

        Hide
        Niclas Hedhman added a comment -

        Feel free to commit yourself...

        Show
        Niclas Hedhman added a comment - Feel free to commit yourself...
        Hide
        Peter Doornbosch added a comment -

        ok

        Show
        Peter Doornbosch added a comment - ok
        Hide
        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 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 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 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
        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
        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 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 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
            Reporter:
            Peter Doornbosch
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development