RFR [15] 8161558: ListIterator should not discard cause on exception
Stuart Marks
stuart.marks at oracle.com
Mon Feb 10 23:57:52 UTC 2020
Hi Remi,
The bug report originally requested that a bunch of different exceptions include
a cause. I don't think the cause should be added to all of them. The cases that
Kiran is adding are ones where I thought that adding a cause does have value.
If someone is using a ListIterator (or a plain Iterator for that matter) they
might have an incorrect model for what the index value is after a certain
operation (remove, for example), and they might get an NSEE unexpectedly. They
might reasonably wonder what the state of the iterator is that resulted in that
exception. Without a cause, NSEE doesn't have that information. Chaining the
IOOBE will usually include the index that caused the problem, which I think is
useful in such circumstances.
The iterators in AbstractList all keep track of indexes and call get() for
access to the appropriate element. I don't think they should do a bounds check
and throw an exception on that basis, because the bounds could change between
the time they're checked and the call to get(). Thus, the iterators would have
to catch the exception from get() even if the bounds are checked in advance,
making the bounds check redundant.
s'marks
On 2/5/20 2:05 PM, Remi Forax wrote:
> Stuart, Martin, Kiran,
> I think this "bug" should not be fixed because it's one of the cases where providing more information is actually bad from a user POV.
>
> The current code throws NoSuchElementException when the iterator reach the end so from the user POV, this is the right exception because of the right issue, so from the user POV there is no need to change the actual code.
> If we chain the exception,
> - it's less clear from a user POV
> - the user may think that there is an error in the AbstractList implementation but it's not the case, it's just that AbstractList iterators next method is implemented weirdly, it prefers to go out of bound instead of checking the bound.
>
> I'm okay with NoSuchElementException having a new constructor that takes a chained exception but i really think that in this specific case, it's a bad idea(TM) to use it to propagate an exception to the user that it should not care about.
>
> BTW, perhaps, those method next() should be re-written to test the bound instead of catching the IOOBE because i'm not sure this "optimization" make sense nowadays.
>
> regards,
> Rémi
>
> ----- Mail original -----
>> De: "Kiran Ravikumar" <kiran.sidhartha.ravikumar at oracle.com>
>> À: "core-libs-dev" <core-libs-dev at openjdk.java.net>
>> Envoyé: Mercredi 5 Février 2020 20:49:09
>> Objet: Re: RFR [15] 8161558: ListIterator should not discard cause on exception
>
>> Thanks Stuart and Martin,
>>
>>
>> Here is an updated webrev with the changes.
>>
>> http://cr.openjdk.java.net/~kravikumar/8161558/webrev.01/
>>
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8161558
>>
>>
>> Thanks,
>>
>> Kiran
>>
>>
>> On 15/01/2020 12:46, Martin Buchholz wrote:
>>> Hi Kiran,
>>>
>>> Looks good to me, but I always nitpick ...
>>>
>>> Stray semicolon?
>>> var iterator = list.listIterator(list.size());; // position at end
>>>
>>> I would have documented whitebox test assumptions: that nCopies
>>> iterators are implemented via AbstractList, and that
>>> AbstractList's list iterator inherits behavior from iterator.
>>>
>>> I probably would have added a plain iterator test, and might have
>>> refactored the code that tests the exception.
>>>
>>>
>>> On Wed, Jan 15, 2020 at 4:07 AM Kiran Ravikumar
>>> <kiran.sidhartha.ravikumar at oracle.com
>>> <mailto:kiran.sidhartha.ravikumar at oracle.com>> wrote:
>>>
>>> Hi Guys,
>>>
>>>
>>> Could someone please review my fix to add missing standard
>>> constructor
>>> overloads to NoSuchElementException class and update the AbstractList
>>> class to use them.
>>>
>>>
>>> A CSR was filed and approved. Along with the code change a new
>>> test is
>>> added to verify the behavior.
>>>
>>>
>>> Please find the webrev at -
>>>
>>>
>>> http://cr.openjdk.java.net/~kravikumar/8161558/webrev.00/
>>>
>>>
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8161558
>>>
>>>
>>> Thanks,
>>>
>>> Kiran
More information about the core-libs-dev
mailing list