[ZODB-Dev] Re: OID length

[Tim Peters]

[Tim]
>>     _p_oid = Attribute(
>>         """The object id.
>> ...
>>         Non-None object ids must be non-empty strings.

[Florent]
> Ok then I guess this comment in Connection doesn't apply anymore:
>
> # It sucks that we have to hold the lock to read _invalidated.
> # Normally, _invalidated is written by calling dict.update, which
> # will execute atomically by virtue of the GIL.  But some storage
> # might generate oids where hash or compare invokes Python code.  In
> # that case, the GIL can't save us.

Possibly, but I don't know.  The specific _reason_ it gives clearly doesn't
apply, but I never take code comments as being wholly accurate, and
especially not when they're claiming to be an exhaustive account of
threading insecurities.  For example, it's easy to imagine that some other
reason(s) for holding the lock also exist(s), but that nobody noticed it
simply because holding the lock has prevented all other possible related
races from occurring.

Mucking with locks is dangerous, and it's rarely clear what the true intent
was.  For example, exactly which subset of the four existing ._inv_lock
release/acquire pairs do you think that comment is talking about?  Did those
four even all exist at the time the comment was written?  Regardless, why do
other accesses of ._invalidated in Connection.py not lock at all?  Is there
code in Zope2 or Zope3 that reaches into Connection and mucks with .inv_lock
and/or ._invalidated too (the leading "I'm private" underscore typically
doesn't inhibit Zope2 coders ;-))?

It generally takes me hours of poking and thinking before I can answer all
questions "like that" with any confidence, and the potential costs of
introducing a new threading problem usually far outweigh the potential
benefits of removing a critical section.  All I could make time for here now
is to add another blurb to that comment, like "Note:  oids are always
strings, so that reasoning may no longer apply; it's unclear.  TODO:  figure
this out, and remove the unnecessary critical sections (if any)."

Of course I wanted to nail the type of oids precisely so that "mystery code"
like this _could_ be cleaned up.  Sometimes progress is slow ;-)