Lucene - Core
  1. Lucene - Core
  2. LUCENE-5532

AutomatonQuery.hashCode is not thread safe

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.7.1, 4.8, 6.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      This hashCode is implemented based on #states and #transitions.

      These methods use getNumberedStates() though, which may oversize itself during construction and then "size down" when its done. But numberedStates is prematurely set (before its "ready"), which can cause a hashCode call from another thread to see a corrupt state... causing things like NPEs from null states and other strangeness. I don't think we should set this variable until its "finished".

      1. LUCENE-5532.patch
        9 kB
        Robert Muir
      2. LUCENE-5532.patch
        9 kB
        Robert Muir

        Activity

        Hide
        Robert Muir added a comment -

        I am also unhappy about this:

              // we already minimized the automaton in the ctor, so
              // this hash code will be the same for automata that
              // are the same:
              int automatonHashCode = automaton.getNumberOfStates() * 3 + automaton.getNumberOfTransitions() * 2;
        

        This comment is out of date! So the whole algorithm is broken in some cases I think.
        I would really prefer if we don't mess with mutable stuff in equals/hashcode.
        I think it would be better if this stuff was impl'ed in CompiledAutomaton? I'll give it a stab.

        Show
        Robert Muir added a comment - I am also unhappy about this: // we already minimized the automaton in the ctor, so // this hash code will be the same for automata that // are the same: int automatonHashCode = automaton.getNumberOfStates() * 3 + automaton.getNumberOfTransitions() * 2; This comment is out of date! So the whole algorithm is broken in some cases I think. I would really prefer if we don't mess with mutable stuff in equals/hashcode. I think it would be better if this stuff was impl'ed in CompiledAutomaton? I'll give it a stab.
        Hide
        Robert Muir added a comment -

        Here's a test for the thread safety bug, fails like this:

        Caused by: java.lang.NullPointerException
        	at org.apache.lucene.util.automaton.Automaton.getNumberOfTransitions(Automaton.java:543)
        	at org.apache.lucene.search.AutomatonQuery.hashCode(AutomatonQuery.java:84)
        	at org.apache.lucene.search.TestAutomatonQuery$1.run(TestAutomatonQuery.java:228)
        

        this patch takes a different approach: it doesnt assert "same language" at all, thats expensive and I don't think its nearly as important as things like being thread-safe, hashcode being consistent with equals, etc.

        So we just impl hashcode/equals with the compiled form. If the automata have the same structure (e.g. same regex or wildcard), it will return true. The previous stuff was overkill anyway, because e.g. foo* would not equate to foo** since the "term" is different!

        I also made getNumberedStates a little less trappy, even though its no longer used here by this stuff.

        Show
        Robert Muir added a comment - Here's a test for the thread safety bug, fails like this: Caused by: java.lang.NullPointerException at org.apache.lucene.util.automaton.Automaton.getNumberOfTransitions(Automaton.java:543) at org.apache.lucene.search.AutomatonQuery.hashCode(AutomatonQuery.java:84) at org.apache.lucene.search.TestAutomatonQuery$1.run(TestAutomatonQuery.java:228) this patch takes a different approach: it doesnt assert "same language" at all, thats expensive and I don't think its nearly as important as things like being thread-safe, hashcode being consistent with equals, etc. So we just impl hashcode/equals with the compiled form. If the automata have the same structure (e.g. same regex or wildcard), it will return true. The previous stuff was overkill anyway, because e.g. foo* would not equate to foo** since the "term" is different! I also made getNumberedStates a little less trappy, even though its no longer used here by this stuff.
        Hide
        Robert Muir added a comment -

        same patch, just with some reordering of things in RunAutomaton.equals for faster speed.

        Show
        Robert Muir added a comment - same patch, just with some reordering of things in RunAutomaton.equals for faster speed.
        Hide
        Simon Willnauer added a comment -

        +1 to the patch - I agree with the change to go away from acceptsSameLanguage! speedups are also good though. Let's make sure we put this on the change runtime behavior section in CHANGES.TXT

        Show
        Simon Willnauer added a comment - +1 to the patch - I agree with the change to go away from acceptsSameLanguage! speedups are also good though. Let's make sure we put this on the change runtime behavior section in CHANGES.TXT
        Hide
        Uwe Schindler added a comment -

        +1 to the patch.

        I am not sure about this, I did not get the latest test-framework updates: It looks to me that the thread does not use a custom name or is the thread group inherited by the test framework? I would also change the thread to simply Assert.fail() on Exception in the thread.

        Show
        Uwe Schindler added a comment - +1 to the patch. I am not sure about this, I did not get the latest test-framework updates: It looks to me that the thread does not use a custom name or is the thread group inherited by the test framework? I would also change the thread to simply Assert.fail() on Exception in the thread.
        Hide
        Robert Muir added a comment -

        Uwe, it just inherits. Its similar to many tests in the .index package that work this way. If we want to do something else, we should ban some methods. But I can name the threads if you want

        As far as Assert.fail, this would lose the stacktrace of the original exception? In the case of this test failing due to a thread safety issue, I think thats useful for debugging

        Show
        Robert Muir added a comment - Uwe, it just inherits. Its similar to many tests in the .index package that work this way. If we want to do something else, we should ban some methods. But I can name the threads if you want As far as Assert.fail, this would lose the stacktrace of the original exception? In the case of this test failing due to a thread safety issue, I think thats useful for debugging
        Hide
        Uwe Schindler added a comment -

        Uwe, it just inherits. Its similar to many tests in the .index package that work this way. If we want to do something else, we should ban some methods. But I can name the threads if you want

        All fine, just wanted to be sure. And we dont have Thread#<init>() on the forbidden list

        As far as Assert.fail, this would lose the stacktrace of the original exception? In the case of this test failing due to a thread safety issue, I think thats useful for debugging

        Yes it will loose. I dont like RuntimeExceptions wrapping others. You can ideally do Rethrow.rethrow(ex). In tests this is I think the preferred way. You will get the original Exception in the thread stack dump, unwrapped. This is why we have Rethrow class in test framework.

        Show
        Uwe Schindler added a comment - Uwe, it just inherits. Its similar to many tests in the .index package that work this way. If we want to do something else, we should ban some methods. But I can name the threads if you want All fine, just wanted to be sure. And we dont have Thread#<init>() on the forbidden list As far as Assert.fail, this would lose the stacktrace of the original exception? In the case of this test failing due to a thread safety issue, I think thats useful for debugging Yes it will loose. I dont like RuntimeExceptions wrapping others. You can ideally do Rethrow.rethrow(ex). In tests this is I think the preferred way. You will get the original Exception in the thread stack dump, unwrapped. This is why we have Rethrow class in test framework.
        Hide
        Robert Muir added a comment -

        Rethrow is good: I'll use that!

        Show
        Robert Muir added a comment - Rethrow is good: I'll use that!
        Hide
        Michael McCandless added a comment -

        +1

        I love the use of startingGun in the test

        Show
        Michael McCandless added a comment - +1 I love the use of startingGun in the test
        Hide
        Dawid Weiss added a comment -

        > or is the thread group inherited by the test framework?

        The test suite runs in its own test group so any thread (unless explicitly assigned to another group) will inherit that group from its parent.

        Show
        Dawid Weiss added a comment - > or is the thread group inherited by the test framework? The test suite runs in its own test group so any thread (unless explicitly assigned to another group) will inherit that group from its parent.
        Hide
        ASF subversion and git services added a comment -

        Commit 1578483 from Robert Muir in branch 'dev/trunk'
        [ https://svn.apache.org/r1578483 ]

        LUCENE-5532: AutomatonQuery.hashCode is not thread safe

        Show
        ASF subversion and git services added a comment - Commit 1578483 from Robert Muir in branch 'dev/trunk' [ https://svn.apache.org/r1578483 ] LUCENE-5532 : AutomatonQuery.hashCode is not thread safe
        Hide
        ASF subversion and git services added a comment -

        Commit 1578491 from Robert Muir in branch 'dev/branches/branch_4x'
        [ https://svn.apache.org/r1578491 ]

        LUCENE-5532: AutomatonQuery.hashCode is not thread safe

        Show
        ASF subversion and git services added a comment - Commit 1578491 from Robert Muir in branch 'dev/branches/branch_4x' [ https://svn.apache.org/r1578491 ] LUCENE-5532 : AutomatonQuery.hashCode is not thread safe
        Hide
        Robert Muir added a comment -

        Should I backport to 4.7.1 branch?

        Show
        Robert Muir added a comment - Should I backport to 4.7.1 branch?
        Hide
        Steve Rowe added a comment -

        Should I backport to 4.7.1 branch?

        +1

        Show
        Steve Rowe added a comment - Should I backport to 4.7.1 branch? +1
        Hide
        ASF subversion and git services added a comment -

        Commit 1578506 from Robert Muir in branch 'dev/branches/lucene_solr_4_7'
        [ https://svn.apache.org/r1578506 ]

        LUCENE-5532: AutomatonQuery.hashCode is not thread safe

        Show
        ASF subversion and git services added a comment - Commit 1578506 from Robert Muir in branch 'dev/branches/lucene_solr_4_7' [ https://svn.apache.org/r1578506 ] LUCENE-5532 : AutomatonQuery.hashCode is not thread safe
        Hide
        Steve Rowe added a comment -

        Bulk close 4.7.1 issues

        Show
        Steve Rowe added a comment - Bulk close 4.7.1 issues

          People

          • Assignee:
            Unassigned
            Reporter:
            Robert Muir
          • Votes:
            0 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development