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

Wrap _listeners access with _realtimeLockQueue in fetchLatestConfig completionHandler #13776

Conversation

yanz-safe
Copy link
Contributor

@yanz-safe yanz-safe commented Oct 3, 2024

Hey there! So you want to contribute to a Firebase SDK?
Before you file this pull request, please read these guidelines:

Discussion

  • Read the contribution guidelines (CONTRIBUTING.md).
  • If this has been discussed in an issue, make sure to link to the issue here.
    If not, go file an issue about this before creating a pull request to discuss.

Issue: #13773

I did not copy the listeners nor use DispatchQueue.sync to keep the same logic and style of code.

Testing

  • Make sure all existing tests in the repository pass after your change.
  • If you fixed a bug or added a feature, add a new test to cover your code.

This change solves data-racing issue when listener can be added at same time when it is enumerated in completion handler.

Please suggest me If I need to add test for this and what would be the best approach for unit testing this?

I can try to add a 1s-2s loop that will add listeners and, in parallel, call config update.

API Changes

  • At this time we cannot accept changes that affect the public API. If you'd like to help
    us make Firebase APIs better, please propose your change in a feature request so that we
    can discuss it together.

Copy link

google-cla bot commented Oct 3, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@yanz-safe
Copy link
Contributor Author

This change solves data-racing issue when listener can be added at same time when it is enumerated in completion handler.

Please suggest me how If I need to add test for this and what would be the best approach for unit testing this?

I can try to add a 1s-2s loop that will add listeners and, in parallel, call config update.

Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

The fix looks good to me. Thanks for the bug report and fix!

@paulb777 paulb777 requested a review from karenyz October 3, 2024 16:09
@paulb777 paulb777 added this to the 11.4.0 - M155 milestone Oct 3, 2024
Copy link
Contributor

@karenyz karenyz left a comment

Choose a reason for hiding this comment

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

Nice catch! Makes me wonder if this block could also be a method (to match the propogateErrors call above that also accesses listeners), but that can be a future refactor :)

@paulb777 paulb777 merged commit 53459a0 into firebase:main Oct 5, 2024
40 checks passed
@yanz-safe yanz-safe deleted the rcnconfigrealtime_listeners_data_racing_fix branch October 7, 2024 06:03
@firebase firebase locked and limited conversation to collaborators Nov 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants