[Python-Dev] RE: [Python-checkins] python/dist/src/Lib weakref.py,1.19,1.20

[Tim Peters]

[redirected to python-dev]

[Tim]
>> Someone review this, please!  Final releases are getting close, Fred
>> (the weakref guy) won't be around until Tuesday, and the pre-patch
>> code can indeed raise spurious RuntimeErrors in the presence of
>> threads or mutating comparison functions.
>>
>> See the bug report for my confusions:  I can't see any reason for why
>> __delitem__ iterated over the keys.

[Raymond Hettinger]
> Until reading the note on threads, I didn't see the error and thought
> the original code was valid because it returned after the deletion
> instead of continuing to loop through iterkeys.

Note that one of the new tests I checked in provoked RuntimeError without
threads.

>> The new one-liner implementation is much faster, can't raise
>> RuntimeError, and should be> better-behaved in all respects wrt threads.

> Yes, that solves the OP's problem.

>> Bugfix candidate for 2.2.3 too, if someone else agrees with this
>> patch.

> The original code does its contortions to avoid raising a KeyError
> whenever the dictionary entry might have disappeared due to the
> ref count falling to zero and then a new, equal key was formed later.

Sorry, I can't picture what you're trying to say.  Show some code?  If in a
weak-keyed dict d I do

    d[k1] = v
    del k1 # last reference, so the dict mutation went away
    del d[k2]  # where k2 happens to be compare equal to what k1 was

then I claim that *should* raise KeyError, and pretty obviously so.  Note
that the other new test I checked in showed that

    del d[whatever]

never raised KeyError before; I can't see how that can be called a feature,
and if someone thinks it was they neglected to document it, or write a test
that failed when I changed the behavior <wink>.

> If the data disappeared, then, I think ref(key) will return None

No, ref(x) never returns None, regardless of what x may be.  It may raise
TypeError if x is not of a weakly referencable type, and it may raise
MemoryError if we don't have enough memory left to construct a weakref, but
those are the only things that can go wrong.

w = ref(x) followed later by w() will return None, iff x has gone away in
the meantime -- maybe that's what you're thinking of.

> which is a bummer because that is then used (in your patch)
> as a lookup key.

If x and y are two weakly-referencable objects (not necessarily distinct)
that compare equal, then

    ref(x) == ref(y)
and
    hash(ref(x)) == hash(ref(y))

so long as both ref(x)() and ref(y)() don't return None (i.e., so long as x
and y are both still alive).

Soo when I map

   del d[k1]

to

   del d.data[ref(k1)]

it will succeed if and only if d.data has a key for a still-live object, and
that key compares equal to k1; else it will raise KeyError (or maybe
TypeError if k1 is a silly key to test in a weak-keyed dict, or MemoryError
if we run out of memory).  That's what I believe it should do.

> The safest approach (until Fred re-appears) is to keep the original
> approach but use keys() instead of iterkeys().  Then, wrap the
> actual deletion in a try / except KeyError to handle a thread
> race to delete the same weakref object.

I'm not clear on what that means.  By "delete the same weakref object", do
you mean that both threads try to do

    del d[k]

with the same k and the same weak-valued dict d?  If so, then I think one of
them *should* see KeyError, exactly the same as if they tried pulling this
trick with a regular dict.

> I'm sure there is a better way and will take another look tomorrow.

Thanks for trying, but I still don't get it.  It would help if you could
show specific code that you believe worked correctly before but is broken
now.  I added two new tests showing what I believe to be code that was
broken before but works now, and no changes to the existing tests were
needed.