ImmutableDB.Iterator: allocate handles in the ResourceRegistry #1547
Conversation
| -- opened. For example, when closing the ImmutableDB/ChainDB from the | ||
| -- main thread, we'll close all open iterators, each of them opened in | ||
| -- a different thread. | ||
| unsafeRelease itEpochKey |
There was a problem hiding this comment.
Comment is wrong (same for other unsafeRelease): the reason this was needed is because in the consensus tests, the protocol threads are spawned in a different ResourceRegistry than the ChainDB, see #1390. We're not going to fix that in this PR, but add a comment: "this is dangerous, because this allows the iterator to be used by a thread not spawned by this registry, so the iterator can disappear before the thread is terminated".
There was a problem hiding this comment.
Add a comment on #1390 about this.
There was a problem hiding this comment.
I'm currently assuming that iteratorClose is sometimes called from a different thread than the one that created that registry. E.G. at a glance, it looks like ChainDB.Impl.implIteratorNext could be called from a spawned thread, and that calls ImmDB.iteratorClose.
So, even if iterators were managed by the ChainDB registry, wouldn't we see ResourceRegistryUsedFromUnknownThread without unsafeRelease? Or is my assumption wrong?
There was a problem hiding this comment.
My comment (in the code) was wrong.
A resource can in fact be released from another thread, as long as that thread has been spawned by the same registry.
There was a problem hiding this comment.
Ah ha! I was conflating the two error conditions in just the same way as the comment.
The name ResourceRegistryUsedFromUnknownThread is not wrong, but it does admit this misinterpretation. ResourceRegistryUsedFromUntrackedThread (or just the full ResourceRegistryUsedFromThreadNotInRegistry) would be clearer in this aspect, in hindsight.
There was a problem hiding this comment.
The name
ResourceRegistryUsedFromUnknownThreadis not wrong, but it does admit this misinterpretation.ResourceRegistryUsedFromUntrackedThread(or just the fullResourceRegistryUsedFromThreadNotInRegistry) would be clearer in this aspect, in hindsight.
Agreed. Good idea!
There was a problem hiding this comment.
I have renamed ResourceRegistryUsedFromUnknownThread to ResourceRegistryUsedFromUntrackedThread (and updated the comments) in a separate commit.
Fixes #1543. The ChainDB.Iterators and ChainDB.Readers use ImmutableDB.Iterators internally. Both take a ResourceRegistry to allocate the ImmutableDB.Iterator in so that the ImmutableDB.Iterator and the open handle it contains is closed when an exception occurs. If an exception occurs at the wrong time while opening an ImmutableDB.Iterator, after opening the handle, we might leak the handle. Fix this by directly allocating the handle in the ResourceRegistry instead of the ImmutableDB.Iterator that refers to the handle.
See the updated docstring of the `ResourceRegistryUsedFromUnknownThread` constructor.
mrBliss
force-pushed
the
mrBliss/fix-1543
branch
from
7b33c4e to
e63a352
Compare
|
bors r+ |
1547: ImmutableDB.Iterator: allocate handles in the ResourceRegistry r=mrBliss a=mrBliss Fixes #1543. The ChainDB.Iterators and ChainDB.Readers use ImmutableDB.Iterators internally. Both take a ResourceRegistry to allocate the ImmutableDB.Iterator in so that the ImmutableDB.Iterator and the open handle it contains is closed when an exception occurs. If an exception occurs at the wrong time while opening an ImmutableDB.Iterator, after opening the handle, we might leak the handle. Fix this by directly allocating the handle in the ResourceRegistry instead of the ImmutableDB.Iterator that refers to the handle. Co-authored-by: Thomas Winant <[email protected]>
Fixes #1543.
The ChainDB.Iterators and ChainDB.Readers use ImmutableDB.Iterators internally. Both take a ResourceRegistry to allocate the ImmutableDB.Iterator in so that the ImmutableDB.Iterator and the open handle it contains is closed when an exception occurs.
If an exception occurs at the wrong time while opening an ImmutableDB.Iterator, after opening the handle, we might leak the handle. Fix this by directly allocating the handle in the ResourceRegistry instead of the ImmutableDB.Iterator that refers to the handle.