Apologies if I have misunderstood the intention of the code, but I was reading through and it seems like a possible bug with releasing the lock too early when using an iterator over a `Metadata` object. I have not looked into what the consequences of this are in the current code, so sorry if I am making a big deal out of nothing. But `Metadata` seems to be a pretty heavily used class so thought it might be worth looking into.

Please disregard this if this was not a bug and the code is behaving as intended.

In metadata.py `Metadata.{}{}iter__` function:

https://github.com/metabrainz/picard/blob/62c25f38497364ea6f1ae5c8ca0da7d309fc0b94/picard/metadata.py#L543-L545

It seems like it is supposed to hold the lock for the whole lifetime of the returned iterator.
What is actually happening is the lock is being acquired and then immediately released as soon as the return statement is encountered (before the iteration even happens), and so the lock is not held at all while the caller/consumer is actually iterating.

It could be changed to:

def __iter__(self):
    with self._lock.lock_for_read():
        yield from self._store

With this change, the lock is held for the lifetime of the iterator.

This behavior is in line with the way `Metadata.items` iterator is implemented.

What actually is the "lifetime" of an iterator?

(note: this is important so that the lock is actually released)

I think until the consumer finishes iterating (exhausts the iterator) or the iterator falls out of scope (even if it was not exhausted). The lock should be released in either case.

I have done a simple test with a fake context manager representing the lock and a fake class with {}iter{} method which acquires the lock similar to the code above, and tried partial iteration and the context manager {}exit{} seems to be called when there are no more references to the iterator (even if it was not exhausted).

There could be problems if a reference to a partially-iterated iterator is kept around after a function ends, and never exhausted - the lock would not be released. Note that this problem also applies to the current `Metadata.items` implementation. But this is probably not happening (it would be pretty strange to use the iterators like this).

So in other words, my suggested change will probably work as expected (as long as the iterators are used sanely). And in any case, the same issue would also apply to the current items() iterator.

A better design might be to change `Metadata` to be a context manager itself, which acquires and releases the lock on {}enter{} or {}exit{} - then caller must use it with `with` if they want to make sure the lock is acquired. In other words, make acquiring/releasing the lock part of the public API.

Hope this is useful, sorry if I am wasting time with a non-issue. Just thought this might help to prevent hard to find threading bugs.