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:30:16 GMT
sorry the 2nd caller is not the subclass of HashMapEntrySet but the
remove() of AbstractMapIterator in LinkedHashMap

On 5/21/08, Tony Wu <wuyuehao@gmail.com> wrote:
> 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
>


-- 
Tony Wu
China Software Development Lab, IBM

Mime
View raw message