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

Tim Peters tim_one@email.msn.com
Sun, 25 May 2003 01:21:22 -0400

[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 .

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.