Commons JXPath
  1. Commons JXPath
  2. JXPATH-49

JXPathContextReferenceImpl not thread safe

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.1 Final
    • Fix Version/s: None
    • Labels:
      None
    • Environment:

      Operating System: All
      Platform: All

      Description

      The cache used to store compiled xpaths in JXPathContextReferenceImpl is not
      thread-safe. Here's the exception that showsup as a result in stress tests.

      Exception Stack Trace: java.util.ConcurrentModificationException
      at java.util.HashMap$HashIterator.nextEntry(HashMap.java:762)
      at java.util.HashMap$EntryIterator.next(HashMap.java:804)
      at
      org.apache.commons.jxpath.ri.JXPathContextReferenceImpl.cleanupCache(JXPathContextReferenceImpl.java:270)

      at
      org.apache.commons.jxpath.ri.JXPathContextReferenceImpl.compileExpression(JXPathContextReferenceImpl.java:252)

      at
      org.apache.commons.jxpath.ri.JXPathContextReferenceImpl.iteratePointers(JXPathContextReferenceImpl.java:493)

        Activity

        Hide
        Prasad Chodavarapu added a comment -

        Created an attachment (id=7648)
        Added synchronization for the "compiled" xpath cache

        Show
        Prasad Chodavarapu added a comment - Created an attachment (id=7648) Added synchronization for the "compiled" xpath cache
        Hide
        Dmitri Plotnikov added a comment -

        This problem was addressed several months ago.

        Show
        Dmitri Plotnikov added a comment - This problem was addressed several months ago.
        Hide
        Prasad Chodavarapu added a comment -

        I do not see the fix in CVS HEAD. Would appreciate if you can point me to the
        fix (specifically, the version in CVS).

        Thanks in advance.

        Show
        Prasad Chodavarapu added a comment - I do not see the fix in CVS HEAD. Would appreciate if you can point me to the fix (specifically, the version in CVS). Thanks in advance.
        Hide
        Prasad Chodavarapu added a comment -

        Mea culpa, I do see it in the HEAD. However, I'm not sure that the fix is correct.

        What you really need is a readers-writer lock at the least. It's not enough to
        just synchronize on the writes. Double-checked locking is also not useful as it
        is widely known that DCL is broken in java. See
        http://www.javaworld.com/javaworld/jw-02-2001/jw-0209-double_p.html

        Show
        Prasad Chodavarapu added a comment - Mea culpa, I do see it in the HEAD. However, I'm not sure that the fix is correct. What you really need is a readers-writer lock at the least. It's not enough to just synchronize on the writes. Double-checked locking is also not useful as it is widely known that DCL is broken in java. See http://www.javaworld.com/javaworld/jw-02-2001/jw-0209-double_p.html
        Hide
        Dmitri Plotnikov added a comment -

        Hmm... But what's the worst that can happen? Is it that you would compile the
        same expression more than once? This map contains soft references anyway, so
        who cares. I don't mind adding all that synchronization, but first I'd like to
        understand why it is needed.

        Show
        Dmitri Plotnikov added a comment - Hmm... But what's the worst that can happen? Is it that you would compile the same expression more than once? This map contains soft references anyway, so who cares. I don't mind adding all that synchronization, but first I'd like to understand why it is needed.
        Hide
        Prasad Chodavarapu added a comment -

        The outcomes are definitely not guaranteed to be always benign. E.g., a reader
        may try to read from a partially written slot, even as it is being written.
        It'll manifest as a random runtime error that's hard to re-produce. In summary,
        correctness of the program cannot be guaranteed.

        Show
        Prasad Chodavarapu added a comment - The outcomes are definitely not guaranteed to be always benign. E.g., a reader may try to read from a partially written slot, even as it is being written. It'll manifest as a random runtime error that's hard to re-produce. In summary, correctness of the program cannot be guaranteed.
        Hide
        Dmitri Plotnikov added a comment -

        All right. You convinced me.

        Show
        Dmitri Plotnikov added a comment - All right. You convinced me.
        Hide
        Dmitri Plotnikov added a comment -

        Re-fixed the bug by adding synchronization on reads, not only on writes, which
        it had before

        Show
        Dmitri Plotnikov added a comment - Re-fixed the bug by adding synchronization on reads, not only on writes, which it had before

          People

          • Assignee:
            Unassigned
            Reporter:
            Prasad Chodavarapu
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development