harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tony Wu" <wuyue...@gmail.com>
Subject Re: [classlib][luni] java/util/HashMap.java bug waiting to happen?
Date Wed, 21 May 2008 06:08:21 GMT
There are two caller of this method in HashMapEntrySet and its
subclass. Both of them check the parameter before passing to this
method, which guarantes that the second condition will never be
reached. If we try to remove a non-existing entry, a
ConcurrentModificationException should be thrown according to spec.

It's not a public API and I can not imagine that it will be used by
other classes/methods in the fufture. If the second condition were
reached one day, there might be something wrong with its caller rather
than this method. IMO I would remove the redundant condition in the
loop.

On 5/21/08, Aleksey Shipilev <aleksey.shipilev@gmail.com> wrote:
> Hi, I would rather go (3) way as inlining would not exhibit additional
> overhead, but (2) would.
>
> Thanks,
> Aleksey.
>
> On Wed, May 21, 2008 at 6:19 AM, Jim Yu <junjie0122@gmail.com> wrote:
> > Hi Oli,
> >
> > I agree with you. (2) will be simple and can prevent potential risk.
> >
> > 2008/5/21 Oliver Deakin <oliver.deakin@googlemail.com>:
> >
> >> Hi Mark,
> >>
> >> Agreed that this could cause problems in the future if it is called from
> >> another method expecting it to behave properly for entries that do not exist
> >> in the HashMap. Three options immediately spring to mind:
> >> 1) Leave it as is - we won't hit the non-existent entry problem at the
> >> moment due to the method caller always ensuring the entry exists.
> >> 2) Add a condition after the while loop to return without altering the
> >> HashMap if m.next is null.
> >> 3) Get rid of the removeEntry() method by inlining it into the calling
> >> method and fixing it up to handle non-existent entries.
> >>
> >> I think (2) is a simple fix, although (3) is also reasonable since there is
> >> only a single call to this method. I think (1) is, as you say, a problem
> >> waiting to happen.
> >> Personally I'd go with (2).
> >>
> >> Regards,
> >> Oliver
> >>
> >>
> >> Mark Hindess wrote:
> >>
> >>> While reviewing Aleksey's improvements in HARMONY-5791, I noticed the
> >>> following lines in HashMap.java (line 682):
> >>>
> >>>    final void removeEntry(Entry<K, V> entry) {
> >>>        int index = entry.origKeyHash & (elementData.length - 1);
> >>>        Entry<K, V> m = elementData[index];
> >>>        if (m == entry) {
> >>>            elementData[index] = entry.next;
> >>>        } else {
> >>> [1]         while (m.next != entry && m.next != null) {
> >>>                m = m.next;
> >>>            }
> >>> [2]         m.next = entry.next;
> >>>
> >>>        }
> >>>        modCount++;
> >>>        elementCount--;
> >>>    }
> >>>
> >>> The while loop at [1] has two conditions.  The first relates to finding
> >>> the entry and the second relates to getting to the end of the linked
> >>> list (i.e. not finding the entry).  Executing the line [2] only really
> >>> makes sense if the first condition was met; if the second was met then
> >>> it might corrupt the list.
> >>>
> >>> As it happens, the only caller of this method in HashMap.java does
> >>> ensure that the entry is present in the HashMap and thus in the linked
> >>> list but it still seems like a problem waiting to happen (or at the very
> >>> least a redundant condition on a while loop).
> >>>
> >>> It isn't broken so I'm not sure what (if anything) to "fix".
> >>>
> >>> Regards,
> >>> -Mark.
> >>>
> >>>
> >>>
> >>>
> >>>
> >>
> >> --
> >> Oliver Deakin
> >> Unless stated otherwise above:
> >> IBM United Kingdom Limited - Registered in England and Wales with number
> >> 741598. Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire
> >> PO6 3AU
> >>
> >>
> >
> >
> > --
> > Best Regards,
> > Jim, Jun Jie Yu
> >
> > China Software Development Lab, IBM
> >
>


-- 
Tony Wu
China Software Development Lab, IBM

Mime
View raw message