Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical 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
        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
        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
        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
        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
        Robert Muir added a comment -

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

        Show
        Robert Muir added a comment - here's the obvious fix, but no test yet.
        Hide
        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
        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
        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
        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
        Uwe Schindler added a comment -

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

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

        Bulk close after release of 3.5

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

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development