Harmony
  1. Harmony
  2. HARMONY-6678

[classlib][concurrent]ArrayIndexOutofBounds exception reported in ConcurrentSkipListSet during load test

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 6.0M3
    • Fix Version/s: 6.0M3
    • Component/s: Classlib
    • Labels:
      None
    • Environment:
      All Operating Systems
    • Patch Info:
      Patch Available
    • Estimated Complexity:
      Moderate

      Description

      An ArrayIndexOutofBoundsException is thrown when the toArray(T[]) method is invoked, under load test with multiple threads. Concurrent updates to the collection results in changing Iterator sizes. As per the Javadocs, the result of the toArray(T[]) should return as much elements possible from the Collection.

      A toArray(T[]) method override has been implemented in ConcurrentSkipListSet and the bounds check has been included in the base class AbstractCollection to prevent this exception.

      1. 002_HARMONY_6678.patch
        7 kB
        Prashanth KS
      2. 001_HARMONY_6678.patch
        2 kB
        Prashanth KS

        Issue Links

          Activity

          Hide
          Mark Hindess added a comment -

          We only have proposed dates for release and no release manager (well maybe we do now). No dates have been confirmed for release and no code freezes have been announced.

          Show
          Mark Hindess added a comment - We only have proposed dates for release and no release manager (well maybe we do now). No dates have been confirmed for release and no code freezes have been announced.
          Hide
          Prashanth KS added a comment -

          I am OK as long as this gets committed. Improvements can be committed after
          adequate testing.


          Regards
          Prashanth

          Show
          Prashanth KS added a comment - I am OK as long as this gets committed. Improvements can be committed after adequate testing. – Regards Prashanth
          Hide
          Regis Xu added a comment -

          while (iterator.hasNext()) {
          if (index == toArray.length)

          { toArray = trimOrAppend(toArray, toArray.length * 2 + 1); }

          for (; index < toArray.length; index++) {
          if (!iterator.hasNext())

          { return trimOrAppend(toArray, index); }

          toArray[index] = iterator.next();
          }
          }

          I think the inner 'for' loop could be avoid.

          Because it's code freeze now, I'll apply patch after 5.0M16 released. Is it OK ?

          Show
          Regis Xu added a comment - while (iterator.hasNext()) { if (index == toArray.length) { toArray = trimOrAppend(toArray, toArray.length * 2 + 1); } for (; index < toArray.length; index++) { if (!iterator.hasNext()) { return trimOrAppend(toArray, index); } toArray [index] = iterator.next(); } } I think the inner 'for' loop could be avoid. Because it's code freeze now, I'll apply patch after 5.0M16 released. Is it OK ?
          Hide
          Prashanth KS added a comment -

          Hi Regis
          Could you explain in detail about your comments?
          1) Could the two level loops be simplified to one level? Which two levels?
          The while loop and the for loop? These levels have been introduced for
          clearly demarcating the array resizing and the copy portions.

          2) I had the same thought about making the trim method generic, but was also
          having a thought on why we should be repeating the code in toArray() to
          toArray(T[]). Moreover, the docs say,

          "If this collection fits in the specified array with room to spare (i.e.,
          the array has more elements than this collection), the element in the array
          immediately following the end of the collection is set to null. (This is
          useful in determining the length of this collection only if the caller
          knows that this collection does not contain any null elements.) "

          If I employ the array resizing technique, then I have to again add logic to
          implement the above, because the arrays are resized perfectly as per the
          above technique. The patch that I have provided takes care of this, though
          it takes an extra copy. I will try to work this out and reduce the copy.

          We can commit this patch for now and probably discuss the improvements of
          these methods in a separate mail chain.

          Thanks
          Prashanth


          Regards
          Prashanth

          Show
          Prashanth KS added a comment - Hi Regis Could you explain in detail about your comments? 1) Could the two level loops be simplified to one level? Which two levels? The while loop and the for loop? These levels have been introduced for clearly demarcating the array resizing and the copy portions. 2) I had the same thought about making the trim method generic, but was also having a thought on why we should be repeating the code in toArray() to toArray(T[]). Moreover, the docs say, "If this collection fits in the specified array with room to spare (i.e., the array has more elements than this collection), the element in the array immediately following the end of the collection is set to null. (This is useful in determining the length of this collection only if the caller knows that this collection does not contain any null elements.) " If I employ the array resizing technique, then I have to again add logic to implement the above, because the arrays are resized perfectly as per the above technique. The patch that I have provided takes care of this, though it takes an extra copy. I will try to work this out and reduce the copy. We can commit this patch for now and probably discuss the improvements of these methods in a separate mail chain. Thanks Prashanth – Regards Prashanth
          Hide
          Regis Xu added a comment -

          For patch 002_HARMONY_6678.patch:

          while (iterator.hasNext()) {
          if (index == toArray.length)

          { toArray = trimOrAppend(toArray, toArray.length * 2 + 1); }

          for (; index < toArray.length; index++) {
          if (!iterator.hasNext())

          { return trimOrAppend(toArray, index); }

          toArray[index] = iterator.next();
          }
          }

          Could the two level loops be simplified to one level?

          toArray(T[]) invoke toArray(), that make toArray(T[]) gets one more array copy operation than toArray(), I think generalizing trimOrAppend, and both toArray() and toArray(T[]) use it can reduce the extra copy.

          Show
          Regis Xu added a comment - For patch 002_HARMONY_6678.patch: while (iterator.hasNext()) { if (index == toArray.length) { toArray = trimOrAppend(toArray, toArray.length * 2 + 1); } for (; index < toArray.length; index++) { if (!iterator.hasNext()) { return trimOrAppend(toArray, index); } toArray [index] = iterator.next(); } } Could the two level loops be simplified to one level? toArray(T[]) invoke toArray(), that make toArray(T[]) gets one more array copy operation than toArray(), I think generalizing trimOrAppend, and both toArray() and toArray(T[]) use it can reduce the extra copy.
          Hide
          Prashanth KS added a comment -

          Changed the AbstractCollection class alone, as per the discussion with Regis.

          Show
          Prashanth KS added a comment - Changed the AbstractCollection class alone, as per the discussion with Regis.

            People

            • Assignee:
              Unassigned
              Reporter:
              Prashanth KS
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:

                Development