[GitHub] metamodel pull request #175: array.set() on a new ArrayList() crashes with i...

[AlexShvedoff <gi...@git.apache.org>]

GitHub user AlexShvedoff opened a pull request:

    https://github.com/apache/metamodel/pull/175

    array.set() on a new ArrayList() crashes with index out of bound

    I'm surprised this works for anyone else, but at least on MacOS JDK 1.8.0_92 having e.g:
    
    List columns = new ArrayList(5);
    columns.set(0, "foo");
    
    ...crashes with an index out of bounds due to the fact that although the desired capacity of 5 has been specified, the size is zero when set() is called and so it crashes because no element exists at index 0.  See https://stackoverflow.com/questions/8896758/initial-size-for-the-arraylist

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/AlexShvedoff/metamodel master

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/metamodel/pull/175.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #175
    
----
commit 89e723d7285caa48a2daa7240a2487ba2d996c99
Author: Alex Shvedoff <al...@isomorphic.com>
Date:   2017-12-15T01:28:13Z

    array.set() on a new ArrayList() crashes with index out of bound
    
    I'm surprised this works for anyone else, but at least on MacOS JDK 1.8.0_92 having e.g:
    
    List columns = new ArrayList(5);
    columns.set(0, "foo");
    
    ...crashes with an index out of bounds due to the fact that although the desired capacity of 5 has been specified, the size is zero when set() is called and so it crashes because no element exists at index 0.  See https://stackoverflow.com/questions/8896758/initial-size-for-the-arraylist

----


---

[GitHub] metamodel issue #175: array.set() on a new ArrayList() crashes with index ou...

[AlexShvedoff <gi...@git.apache.org>]

Github user AlexShvedoff commented on the issue:

    https://github.com/apache/metamodel/pull/175
  
    Yeah, it looks like previously there was a Columns[] preallocation.  May want to scan the code for other such conversions...
    
    In terms of unit tests, is there really nothing already that covers a basic query?  Because the bug is triggered as soon as you have a select of any sort, even with just one column being selected.  
    
    Maybe the problem is there's not connectivity of automated tests to SFDC?  That would make sense since that requires a login etc...


---

[GitHub] metamodel issue #175: array.set() on a new ArrayList() crashes with index ou...

[kaspersorensen <gi...@git.apache.org>]

Github user kaspersorensen commented on the issue:

    https://github.com/apache/metamodel/pull/175
  
    I checked for any other occurrences (by searching all calls to `.set(...)` and this seems to be the only one that is broken.
    I'll merge it in now.


---

[GitHub] metamodel issue #175: array.set() on a new ArrayList() crashes with index ou...

[kaspersorensen <gi...@git.apache.org>]

Github user kaspersorensen commented on the issue:

    https://github.com/apache/metamodel/pull/175
  
    Hmm this issue is in deed strange. I imagine this bug was introduced in v 5.0 as we replaced arrays with collections in a bunch of places. Maybe this particular place isn't tested. Which I would then suggest we do ... Can this PR be enriched with a unit test that exercises the code?


---

[GitHub] metamodel issue #175: array.set() on a new ArrayList() crashes with index ou...

[AlexShvedoff <gi...@git.apache.org>]

Github user AlexShvedoff commented on the issue:

    https://github.com/apache/metamodel/pull/175
  
    It doesn't look like .add is threadsafe.  Per the ArrayList overview docs, you might be left with the impression that as long as sufficient max capacity has been pre alloc'd you're thread-safe.  But if you look at the implementation of .add:
    
    457     public boolean More ...add(E e) {
    458         ensureCapacityInternal(size + 1);  // Increments modCount!!
    459         elementData[size++] = e;
    460         return true;
    461     }
    
    That elementData[size++] is definitely not atomic, so it's possible for concurrent threads to step on each other and overwrite the same array slot.
    
    But yeah, we are ok here since the ArrayList being used is definitely not shared between threads.


---

[GitHub] metamodel pull request #175: array.set() on a new ArrayList() crashes with i...

[asfgit <gi...@git.apache.org>]

Github user asfgit closed the pull request at:

    https://github.com/apache/metamodel/pull/175


---