Lucene - Core
  1. Lucene - Core
  2. LUCENE-1805

CloseableThreadLocal should allow null Objects

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.1
    • Fix Version/s: 2.9
    • Component/s: modules/analysis
    • Labels:
      None
    • Lucene Fields:
      New, Patch Available

      Description

      CloseableThreadLocal does not allow null Objects in its get() method, but does nothing to prevent them in set(Object). The comment in get() before assert v != null is irrelevant - the application might have passed null.

      Null is an important value for Analyzers. Since tokenStreams (a ThreadLocal private member in Analyzer) is not accessible by extending classes, the only way for an Analyzer to reset the tokenStreams is by calling setPreviousTokenStream(null).

      I will post a patch w/ a test

      1. LUCENE-1805.patch
        2 kB
        Michael McCandless
      2. LUCENE-1805.patch
        2 kB
        Shai Erera

        Activity

        Hide
        Shai Erera added a comment -

        Remove assert v != null and added a test case.

        Funny, but it's as if CTL itself could cause this bug. If you call ctl.get() w/o setting anything first, it creates a WeakReference and set the initValue(), which is ... null !. The first call to get() will return null, the second would fail.

        BTW, TestCloseableThreadLocal is under o.a.l.index. Shouldn't it be under o.a.l.util? Can whoever assigns this to himself move it?

        Show
        Shai Erera added a comment - Remove assert v != null and added a test case. Funny, but it's as if CTL itself could cause this bug. If you call ctl.get() w/o setting anything first, it creates a WeakReference and set the initValue(), which is ... null !. The first call to get() will return null, the second would fail. BTW, TestCloseableThreadLocal is under o.a.l.index. Shouldn't it be under o.a.l.util? Can whoever assigns this to himself move it?
        Hide
        Michael McCandless added a comment -

        If you call ctl.get() w/o setting anything first, it creates a WeakReference and set the initValue(), which is ... null

        I don't think that's right?

        It calls initialValue(), and only if that returns a non-null value will it set it. Else it doesn't call set? (So calling get() twice in a row not having called set shouldn't hit an exception). I think?

        Show
        Michael McCandless added a comment - If you call ctl.get() w/o setting anything first, it creates a WeakReference and set the initValue(), which is ... null I don't think that's right? It calls initialValue(), and only if that returns a non-null value will it set it. Else it doesn't call set? (So calling get() twice in a row not having called set shouldn't hit an exception). I think?
        Hide
        Shai Erera added a comment -

        Oops, you're right. I overlooked it. So maybe the second test case I added is not that important, even though it wouldn't hurt to keep it.

        Show
        Shai Erera added a comment - Oops, you're right. I overlooked it. So maybe the second test case I added is not that important, even though it wouldn't hurt to keep it.
        Hide
        Michael McCandless added a comment -

        Attached small tweaks to the test (removed the misleading comment).

        I think it's ready to commit. If it looks ok Shai I'll commit shortly...

        Show
        Michael McCandless added a comment - Attached small tweaks to the test (removed the misleading comment). I think it's ready to commit. If it looks ok Shai I'll commit shortly...
        Hide
        Shai Erera added a comment -

        I was just commenting that I should remove the misleading comment .

        It looks good. Don't you want to move the test to o.a.l.util as well?

        Show
        Shai Erera added a comment - I was just commenting that I should remove the misleading comment . It looks good. Don't you want to move the test to o.a.l.util as well?
        Hide
        Michael McCandless added a comment -

        Don't you want to move the test to o.a.l.util as well?

        Woops, right, I'll do that before committing.

        Show
        Michael McCandless added a comment - Don't you want to move the test to o.a.l.util as well? Woops, right, I'll do that before committing.
        Hide
        Michael McCandless added a comment -

        Thanks Shai!

        Show
        Michael McCandless added a comment - Thanks Shai!

          People

          • Assignee:
            Michael McCandless
            Reporter:
            Shai Erera
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development