Details

    • Type: Bug
    • Status: Closed
    • Priority: Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 3.5, 4.0-ALPHA
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      this looks really broken/dangerous as an instance variable.

      what happens on clone() ?! copyBytes can instead make its own array inside the method.

      its protected, so ill list in the 3.x backwards breaks section since its technically a backwards break.

      1. LUCENE-3541.patch
        3 kB
        Robert Muir
      2. LUCENE-3541.patch
        3 kB
        Robert Muir
      3. LUCENE-3541.patch
        0.9 kB
        Robert Muir

        Activity

        Hide
        thetaphi Uwe Schindler added a comment -

        It's horrible: When you clone this IndexInput and then in another thread start another copy operation, both will override the others buffer contents. What's the reason for having this as instance variable. If you create the byte[] in the method and it's unused afterwards it happily resides in eden space....

        Show
        thetaphi Uwe Schindler added a comment - It's horrible: When you clone this IndexInput and then in another thread start another copy operation, both will override the others buffer contents. What's the reason for having this as instance variable. If you create the byte[] in the method and it's unused afterwards it happily resides in eden space....
        Hide
        rcmuir Robert Muir added a comment -

        I agree its horrible, if someone properly clone()'s indexinputs and uses threads they are sharing this same buffer and its just asking for corrumption.

        Show
        rcmuir Robert Muir added a comment - I agree its horrible, if someone properly clone()'s indexinputs and uses threads they are sharing this same buffer and its just asking for corrumption.
        Hide
        rcmuir Robert Muir added a comment -

        here's the obvious fix, but no test yet.

        Show
        rcmuir Robert Muir added a comment - here's the obvious fix, but no test yet.
        Hide
        rcmuir Robert Muir added a comment -

        here's a prototype test (it needs to be toned down, but it does the trick), that fails without the patch, passes with it.

        it will only fail with mmapdirectory, all the other core directories override this method and have some custom implementation already.

        Show
        rcmuir Robert Muir added a comment - here's a prototype test (it needs to be toned down, but it does the trick), that fails without the patch, passes with it. it will only fail with mmapdirectory, all the other core directories override this method and have some custom implementation already.
        Hide
        rcmuir Robert Muir added a comment -

        toned down the test, it still fails with mmapdir without the patch.

        i think this is ready to commit.

        Show
        rcmuir Robert Muir added a comment - toned down the test, it still fails with mmapdir without the patch. i think this is ready to commit.
        Hide
        thetaphi Uwe Schindler added a comment -

        +1, commit this as fast as possible, otherwise I will make this a Blocker g

        Show
        thetaphi Uwe Schindler added a comment - +1, commit this as fast as possible, otherwise I will make this a Blocker g
        Hide
        thetaphi Uwe Schindler added a comment -

        Bulk close after release of 3.5

        Show
        thetaphi Uwe Schindler added a comment - Bulk close after release of 3.5

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development