Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: whereField creates a memory leak #12972

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

milaGGL
Copy link
Contributor

@milaGGL milaGGL commented May 15, 2024

fix: #12613
revert the thread safe memoizer introduced in this pr: #11781

@cherylEnkidu :
This PR intends to fix the data race and memory leak issue.

When using memorized field, it leads to data race. However, the solution for fixing data race issue introduces memory leak.

So this PR construct a flatten filters for composite filter during initialization.

The reason for not providing GetFlattenFilter() in base filter class is creating flatten filter in field filter constructor will cause infinity loop.

Since GetFlattenFilter() is widely used in query calls, init flatten filters vector in composite filter during construction won't hurt efficiency a lot comparing to construct flatten filters vector upon request.

@google-oss-bot
Copy link

google-oss-bot commented May 15, 2024

Coverage Report 1

Affected Products

  • FirebaseFirestore-iOS-FirebaseFirestoreInternal.framework

    Overall coverage changed from 88.21% (eca84fd) to 88.19% (9b34ba4) by -0.02%.

    FilenameBase (eca84fd)Merge (9b34ba4)Diff
    composite_filter.cc90.10%89.25%-0.85%
    field_filter.cc95.00%94.78%-0.22%
    filter.cc70.00%62.50%-7.50%
    grpc_stream.cc99.01%98.02%-0.99%
    leveldb_key.cc98.63%98.43%-0.20%
    query.cc98.43%98.33%-0.11%
    query_core.cc95.80%95.81%+0.01%
    target.cc96.23%96.24%+0.01%
    task.cc94.78%96.52%+1.74%

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/nbwANxnpho.html

@cherylEnkidu
Copy link
Contributor

Hi @ehsannas , could you please review this fix?

Copy link
Contributor

@ehsannas ehsannas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason for not providing GetFlattenFilter() in base filter class is creating flatten filter in field filter constructor will cause infinity loop.

why? maybe you shouldn't call GetFlattenedFilters() in the constructor, but should still have GetFlattenedFilters() defined for field filters [which just returns *this].

@ehsannas ehsannas assigned cherylEnkidu and unassigned ehsannas May 29, 2024
@cherylEnkidu
Copy link
Contributor

The reason for not providing GetFlattenFilter() in base filter class is creating flatten filter in field filter constructor will cause infinity loop.

why? maybe you shouldn't call GetFlattenedFilters() in the constructor, but should still have GetFlattenedFilters() defined for field filters [which just returns *this].

@ehsannas I put back GetFlattenedFilters() in the Filter class and return a shared pointer to vector.

@cherylEnkidu cherylEnkidu assigned ehsannas and unassigned cherylEnkidu Jul 3, 2024
// field filter, calling for (const FieldFilter& subFilter :
// *filter.GetFlattenedFilters()) will deference a temporary object.
auto flattened_filters_ptr = filter.GetFlattenedFilters();
for (const FieldFilter& field_filter : *flattened_filters_ptr) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  // Don't deference filter.GetFlattenedFilters() directly. if the filter is a
  // field filter, calling for (const FieldFilter& subFilter :
  // *filter.GetFlattenedFilters()) will deference a temporary object.

This is the result of bad design and devs can't be expected to "do the right thing" by reading the comment.

why can't field filters follow the same pattern as composite filters to avoid this problem?

please reach out offline if you think more discussion is needed. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GetDocuments: whereField creates a memory leak.
4 participants