fix: Panic with different composite-indexed child objects #2947
Conversation
islamaliev
added
bug
islamaliev
changed the title
To me this makes it sound like the previous index iterator should be closed on |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2947 +/- ##
===========================================
- Coverage 79.26% 79.25% -0.02%
===========================================
Files 326 326
Lines 24870 24870
===========================================
- Hits 19713 19709 -4
Misses 3740 3740
- Partials 1417 1421 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 20 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Merge 2 index iterators into one.
| func (iter *indexPrefixIterator) Init(ctx context.Context, store datastore.DSReaderWriter) error { | ||
| iter.ctx = ctx | ||
| iter.store = store | ||
| iter.resultIter = nil |
There was a problem hiding this comment.
suggestion: If iter.resultIter is not nil here, it looks like it should probably be closed before clearing the ref?
I have a vague memory of the normal fetcher failing because of this some time ago, but I might be wrong.
| // create multinode | ||
| multinode := ¶llelNode{ | ||
| // create parallelNode | ||
| parallelNode := ¶llelNode{ |
There was a problem hiding this comment.
praise: I forgot to add to my earlier review, thanks for renaming this.
| // | ||
| // NOTE: calling Init() on multiScanNode is subject to counting as well and as such | ||
| // doesn't not provide idempotency guarantees. Counting is purely for performance | ||
| // reasons and removing it should be safe. |
There was a problem hiding this comment.
praise: This is very clear, thanks Islam :)
| // The issue was that [multiScanNode] that keeps track of how many calls to [Next()] should | ||
| // be made, would call [Init()] and [Start()] every time without tracking, which would result | ||
| // in child nodes being initialized and started multiple times, which in turn created | ||
| // index iterators without closing them. |
There was a problem hiding this comment.
praise: Thanks for taking the time to link this to the ticket, it highlights the importance of the test very nicely.
Relevant issue(s)
Resolves #2924
Description
Make multiScanNode keep track of all calls.
The issue was that multiScanNode that keeps track of how many calls to Next() should be made, would call Init() and Start() every time without tracking, which would result in child nodes being initialized and started multiple times, which in turn created index iterators without closing them.