Lucene - Core
  1. Lucene - Core
  2. LUCENE-3420

assertion derived class modifier from parent class silently breaks backward compatibility

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Won't Fix
    • Affects Version/s: 3.3
    • Fix Version/s: None
    • Component/s: core/index
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      after upgrading to lucene 3.1+, I see this in my log:

      java.lang.AssertionError: TokenStream implementation classes or at least their incrementToken() implementation must be final
      at org.apache.lucene.analysis.TokenStream.assertFinal(TokenStream.java:117)
      at org.apache.lucene.analysis.TokenStream.<init>(TokenStream.java:92)

      Turns out I derived TokenStream and my class was not declared final.

      This silently breaks backward compatibility via reflection, scary...

      I think doing this sort of check is fine, but throwing an java.lang.AssertionError in this case is too stringent.

      This is a style check against lucene clients, a error log would be fine, but throwing an Error is too much.

      See constructor implementation for: http://svn.apache.org/viewvc/lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/analysis/TokenStream.java?view=markup

      1. LUCENE-3420.patch
        1 kB
        Uwe Schindler

        Activity

        Hide
        Yonik Seeley added a comment -

        Hmmm, yeah. Perhaps this assert should have only been for token streams in our namespace?

        Show
        Yonik Seeley added a comment - Hmmm, yeah. Perhaps this assert should have only been for token streams in our namespace?
        Hide
        Robert Muir added a comment -

        This silently breaks backward compatibility via reflection, scary...

        This is not true, there is nothing silent about it. It was listed in the backwards compatibility breaks section of 3.1:

          Analyzer and TokenStream base classes now have an assertion in their ctor,
          that check subclasses to be final or at least have final implementations
          of incrementToken(), tokenStream(), and reusableTokenStream().
        
        Show
        Robert Muir added a comment - This silently breaks backward compatibility via reflection, scary... This is not true, there is nothing silent about it. It was listed in the backwards compatibility breaks section of 3.1: Analyzer and TokenStream base classes now have an assertion in their ctor, that check subclasses to be final or at least have final implementations of incrementToken(), tokenStream(), and reusableTokenStream().
        Hide
        Uwe Schindler added a comment -

        The problem behind the checks is that they are done by the class TokenStream, so even if you disable assertions for your own class this will still fail as soon as you enable assertions for the lucene package.

        If you want to enable assertions for Lucene but disable assertions in your own code, the ctor should check the actual assertion status of the subclass using Class.desiredAssertionStatus() and not throw AssertionFailedError, as this would affect a class for which assertions are not enabled. Patch is easy.

        The same applies for Analyzer.

        In another issue (sorry, I don't find it, the JIRA search is dumb - oh f*ck it's Lucene! - maybe it was on mailing list), Shai Erea suggested to do the assertion check only for org.apache.lucene and org.apache.solr packages. There is already a patch somewhere - if I could find the issue.

        Show
        Uwe Schindler added a comment - The problem behind the checks is that they are done by the class TokenStream, so even if you disable assertions for your own class this will still fail as soon as you enable assertions for the lucene package. If you want to enable assertions for Lucene but disable assertions in your own code, the ctor should check the actual assertion status of the subclass using Class.desiredAssertionStatus() and not throw AssertionFailedError, as this would affect a class for which assertions are not enabled. Patch is easy. The same applies for Analyzer. In another issue (sorry, I don't find it, the JIRA search is dumb - oh f*ck it's Lucene! - maybe it was on mailing list), Shai Erea suggested to do the assertion check only for org.apache.lucene and org.apache.solr packages. There is already a patch somewhere - if I could find the issue.
        Hide
        John Wang added a comment -

        Maybe it is just my lack of understanding why the assert is needed. My ignorance tells me it is for code styling. I am sure there is something deeper. Can someone enlighten me?

        Show
        John Wang added a comment - Maybe it is just my lack of understanding why the assert is needed. My ignorance tells me it is for code styling. I am sure there is something deeper. Can someone enlighten me?
        Hide
        Uwe Schindler added a comment -

        Most of this is explained in the related issues to LUCENE-2389.

        For analyzers enforcing finalness prevents errors in handling the two different tokenStream() methods (for reuse and not reuse). Also the problems in Lucene 2.9 to make the new TokenStream API backwards compatible (the famous "sophisticated backwards layer") lead to enforcing finalness for this API using the decorator pattern. With this assertion we found lots of bugs even in Lucene code that were caused by overriding methods to change behaviour that should never be done in that way for decorator APIs.

        And this is NOT a HARD error. No production system is broken by that, as assertions are generally only enabled for testing. So the assertion failure simply tells your test system that you have to fix your code. In production without assertions, your code will still work. So where is the problem?

        If you enable assertions on a production system you will see significant performance problems!!!

        Show
        Uwe Schindler added a comment - Most of this is explained in the related issues to LUCENE-2389 . For analyzers enforcing finalness prevents errors in handling the two different tokenStream() methods (for reuse and not reuse). Also the problems in Lucene 2.9 to make the new TokenStream API backwards compatible (the famous "sophisticated backwards layer") lead to enforcing finalness for this API using the decorator pattern. With this assertion we found lots of bugs even in Lucene code that were caused by overriding methods to change behaviour that should never be done in that way for decorator APIs. And this is NOT a HARD error. No production system is broken by that, as assertions are generally only enabled for testing. So the assertion failure simply tells your test system that you have to fix your code. In production without assertions, your code will still work. So where is the problem? If you enable assertions on a production system you will see significant performance problems!!!
        Hide
        Uwe Schindler added a comment -

        Here a patch that the assertion status of the actual class is used. If you disable assertions for your own code, but leave assertions in Lucene enabled, the failure will not trigger. This is the more correct approach. The reflection check should use the subclass' assertion status to enable/disable the check.

        Show
        Uwe Schindler added a comment - Here a patch that the assertion status of the actual class is used. If you disable assertions for your own code, but leave assertions in Lucene enabled, the failure will not trigger. This is the more correct approach. The reflection check should use the subclass' assertion status to enable/disable the check.
        Hide
        Uwe Schindler added a comment -

        Hi, any comments? I would like to commit the "change" (it's not a fix as nothing is borken!) to trunk and 3.x,

        Show
        Uwe Schindler added a comment - Hi, any comments? I would like to commit the "change" (it's not a fix as nothing is borken!) to trunk and 3.x,
        Hide
        Uwe Schindler added a comment -

        Committed change to trunk revision: 1172227; 3.x revision: 1172228

        Show
        Uwe Schindler added a comment - Committed change to trunk revision: 1172227; 3.x revision: 1172228
        Hide
        Uwe Schindler added a comment -

        This is not a problem, as this change was announced in the backwards compatibility section of Lucene version 3.1

        Show
        Uwe Schindler added a comment - This is not a problem, as this change was announced in the backwards compatibility section of Lucene version 3.1

          People

          • Assignee:
            Uwe Schindler
            Reporter:
            John Wang
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development