From a9cb8779c39d348f2114ae1f4c43806ee5cdda06 Mon Sep 17 00:00:00 2001 From: Steve Yen Date: Thu, 29 Sep 2016 16:51:42 -0700 Subject: [PATCH 1/2] more careful Close()'ing and cleanup of searchers From diagnosing a recent issue where the termSearchersFinished stats were incorrectly tracked, I ended up scouring the Close() / cleanup codepaths. This change takes more care in Close()'ing child searchers, especially in error situations. This can be important to allow underlying kvstore's to release resources. --- search/query/conjunction.go | 5 +++++ search/query/disjunction.go | 5 +++++ search/searchers/search_boolean.go | 25 ++++++++++++------------ search/searchers/search_conjunction.go | 8 ++++---- search/searchers/search_disjunction.go | 8 ++++---- search/searchers/search_docid.go | 2 +- search/searchers/search_fuzzy.go | 8 +++++++- search/searchers/search_match_all.go | 1 + search/searchers/search_numeric_range.go | 9 +++++++++ search/searchers/search_regexp.go | 8 +++++++- search/searchers/search_term.go | 1 + 11 files changed, 57 insertions(+), 23 deletions(-) diff --git a/search/query/conjunction.go b/search/query/conjunction.go index 32ab7b4f..588d908b 100644 --- a/search/query/conjunction.go +++ b/search/query/conjunction.go @@ -48,6 +48,11 @@ func (q *ConjunctionQuery) Searcher(i index.IndexReader, m mapping.IndexMapping, var err error ss[in], err = conjunct.Searcher(i, m, explain) if err != nil { + for _, searcher := range ss { + if searcher != nil { + _ = searcher.Close() + } + } return nil, err } } diff --git a/search/query/disjunction.go b/search/query/disjunction.go index 7008d9ad..9bc8b79c 100644 --- a/search/query/disjunction.go +++ b/search/query/disjunction.go @@ -54,6 +54,11 @@ func (q *DisjunctionQuery) Searcher(i index.IndexReader, m mapping.IndexMapping, var err error ss[in], err = disjunct.Searcher(i, m, explain) if err != nil { + for _, searcher := range ss { + if searcher != nil { + _ = searcher.Close() + } + } return nil, err } } diff --git a/search/searchers/search_boolean.go b/search/searchers/search_boolean.go index 8a94929a..5bb6e81d 100644 --- a/search/searchers/search_boolean.go +++ b/search/searchers/search_boolean.go @@ -345,23 +345,24 @@ func (s *BooleanSearcher) Count() uint64 { } func (s *BooleanSearcher) Close() error { + var err0, err1, err2 error if s.mustSearcher != nil { - err := s.mustSearcher.Close() - if err != nil { - return err - } + err0 = s.mustSearcher.Close() } if s.shouldSearcher != nil { - err := s.shouldSearcher.Close() - if err != nil { - return err - } + err1 = s.shouldSearcher.Close() } if s.mustNotSearcher != nil { - err := s.mustNotSearcher.Close() - if err != nil { - return err - } + err2 = s.mustNotSearcher.Close() + } + if err0 != nil { + return err0 + } + if err1 != nil { + return err1 + } + if err2 != nil { + return err2 } return nil } diff --git a/search/searchers/search_conjunction.go b/search/searchers/search_conjunction.go index c9fe0f89..34313b73 100644 --- a/search/searchers/search_conjunction.go +++ b/search/searchers/search_conjunction.go @@ -204,14 +204,14 @@ func (s *ConjunctionSearcher) Count() uint64 { return sum } -func (s *ConjunctionSearcher) Close() error { +func (s *ConjunctionSearcher) Close() (rv error) { for _, searcher := range s.searchers { err := searcher.Close() - if err != nil { - return err + if err != nil && rv == nil { + rv = err } } - return nil + return rv } func (s *ConjunctionSearcher) Min() int { diff --git a/search/searchers/search_disjunction.go b/search/searchers/search_disjunction.go index daa9f33b..51772dc0 100644 --- a/search/searchers/search_disjunction.go +++ b/search/searchers/search_disjunction.go @@ -215,14 +215,14 @@ func (s *DisjunctionSearcher) Count() uint64 { return sum } -func (s *DisjunctionSearcher) Close() error { +func (s *DisjunctionSearcher) Close() (rv error) { for _, searcher := range s.searchers { err := searcher.Close() - if err != nil { - return err + if err != nil && rv == nil { + rv = err } } - return nil + return rv } func (s *DisjunctionSearcher) Min() int { diff --git a/search/searchers/search_docid.go b/search/searchers/search_docid.go index 33b9b4c9..86c2b02f 100644 --- a/search/searchers/search_docid.go +++ b/search/searchers/search_docid.go @@ -76,7 +76,7 @@ func (s *DocIDSearcher) Advance(ctx *search.SearchContext, ID index.IndexInterna } func (s *DocIDSearcher) Close() error { - return nil + return s.reader.Close() } func (s *DocIDSearcher) Min() int { diff --git a/search/searchers/search_fuzzy.go b/search/searchers/search_fuzzy.go index 2cd44ebf..a433177c 100644 --- a/search/searchers/search_fuzzy.go +++ b/search/searchers/search_fuzzy.go @@ -39,10 +39,15 @@ func NewFuzzySearcher(indexReader index.IndexReader, term string, prefix, fuzzin // enumerate all the terms in the range qsearchers := make([]search.Searcher, 0, len(candidateTerms)) - + qsearchersClose := func() { + for _, searcher := range qsearchers { + _ = searcher.Close() + } + } for _, cterm := range candidateTerms { qsearcher, err := NewTermSearcher(indexReader, cterm, field, boost, explain) if err != nil { + qsearchersClose() return nil, err } qsearchers = append(qsearchers, qsearcher) @@ -51,6 +56,7 @@ func NewFuzzySearcher(indexReader index.IndexReader, term string, prefix, fuzzin // build disjunction searcher of these ranges searcher, err := NewDisjunctionSearcher(indexReader, qsearchers, 0, explain) if err != nil { + qsearchersClose() return nil, err } diff --git a/search/searchers/search_match_all.go b/search/searchers/search_match_all.go index 5659f88b..3908670a 100644 --- a/search/searchers/search_match_all.go +++ b/search/searchers/search_match_all.go @@ -29,6 +29,7 @@ func NewMatchAllSearcher(indexReader index.IndexReader, boost float64, explain b } count, err := indexReader.DocCount() if err != nil { + _ = reader.Close() return nil, err } scorer := scorers.NewConstantScorer(1.0, boost, explain) diff --git a/search/searchers/search_numeric_range.go b/search/searchers/search_numeric_range.go index 30e7f55b..1d701403 100644 --- a/search/searchers/search_numeric_range.go +++ b/search/searchers/search_numeric_range.go @@ -62,16 +62,25 @@ func NewNumericRangeSearcher(indexReader index.IndexReader, min *float64, max *f } // enumerate all the terms in the range qsearchers := make([]search.Searcher, len(terms)) + qsearchersClose := func() { + for _, searcher := range qsearchers { + if searcher != nil { + _ = searcher.Close() + } + } + } for i, term := range terms { var err error qsearchers[i], err = NewTermSearcher(indexReader, string(term), field, boost, explain) if err != nil { + qsearchersClose() return nil, err } } // build disjunction searcher of these ranges searcher, err := NewDisjunctionSearcher(indexReader, qsearchers, 0, explain) if err != nil { + qsearchersClose() return nil, err } return &NumericRangeSearcher{ diff --git a/search/searchers/search_regexp.go b/search/searchers/search_regexp.go index 3ce61d82..e6cedec1 100644 --- a/search/searchers/search_regexp.go +++ b/search/searchers/search_regexp.go @@ -41,10 +41,15 @@ func NewRegexpSearcher(indexReader index.IndexReader, pattern *regexp.Regexp, fi // enumerate all the terms in the range qsearchers := make([]search.Searcher, 0, len(candidateTerms)) - + qsearchersClose := func() { + for _, searcher := range qsearchers { + _ = searcher.Close() + } + } for _, cterm := range candidateTerms { qsearcher, err := NewTermSearcher(indexReader, cterm, field, boost, explain) if err != nil { + qsearchersClose() return nil, err } qsearchers = append(qsearchers, qsearcher) @@ -53,6 +58,7 @@ func NewRegexpSearcher(indexReader index.IndexReader, pattern *regexp.Regexp, fi // build disjunction searcher of these ranges searcher, err := NewDisjunctionSearcher(indexReader, qsearchers, 0, explain) if err != nil { + qsearchersClose() return nil, err } diff --git a/search/searchers/search_term.go b/search/searchers/search_term.go index fcdf6ccd..f4a727c7 100644 --- a/search/searchers/search_term.go +++ b/search/searchers/search_term.go @@ -32,6 +32,7 @@ func NewTermSearcher(indexReader index.IndexReader, term string, field string, b } count, err := indexReader.DocCount() if err != nil { + _ = reader.Close() return nil, err } scorer := scorers.NewTermQueryScorer(term, field, boost, count, reader.Count(), explain) From c362ab302e2949260aaedb60b8a7de531a55aaad Mon Sep 17 00:00:00 2001 From: Steve Yen Date: Fri, 30 Sep 2016 14:08:04 -0700 Subject: [PATCH 2/2] fix tracking of termSearchersFinished stats --- index/smolder/reader.go | 4 +++- index/upsidedown/reader.go | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/index/smolder/reader.go b/index/smolder/reader.go index 419f7c07..0fa517d9 100644 --- a/index/smolder/reader.go +++ b/index/smolder/reader.go @@ -122,6 +122,9 @@ func (r *SmolderingCouchTermFieldReader) Advance(docID index.IndexInternalID, pr } func (r *SmolderingCouchTermFieldReader) Close() error { + if r.indexReader != nil { + atomic.AddUint64(&r.indexReader.index.stats.termSearchersFinished, uint64(1)) + } if r.iterator != nil { return r.iterator.Close() } @@ -255,6 +258,5 @@ func (r *SmolderingCouchDocIDReader) Advance(docID index.IndexInternalID) (index } func (r *SmolderingCouchDocIDReader) Close() error { - atomic.AddUint64(&r.indexReader.index.stats.termSearchersFinished, uint64(1)) return r.iterator.Close() } diff --git a/index/upsidedown/reader.go b/index/upsidedown/reader.go index cb64899c..19ffe87f 100644 --- a/index/upsidedown/reader.go +++ b/index/upsidedown/reader.go @@ -141,6 +141,9 @@ func (r *UpsideDownCouchTermFieldReader) Advance(docID index.IndexInternalID, pr } func (r *UpsideDownCouchTermFieldReader) Close() error { + if r.indexReader != nil { + atomic.AddUint64(&r.indexReader.index.stats.termSearchersFinished, uint64(1)) + } if r.iterator != nil { return r.iterator.Close() } @@ -297,7 +300,6 @@ func (r *UpsideDownCouchDocIDReader) Advance(docID index.IndexInternalID) (index } func (r *UpsideDownCouchDocIDReader) Close() error { - atomic.AddUint64(&r.indexReader.index.stats.termSearchersFinished, uint64(1)) return r.iterator.Close() }