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

Synchronize Reader Search adapter #10386

Merged
merged 1 commit into from
Aug 20, 2019
Merged

Conversation

malinajirka
Copy link
Contributor

Fixes #9987

This fix is more of a shot in the dark. We have been seeing this ChangeObserver is already registered. exception for the last couple of years and we haven't been able to find steps how to reproduce it.

I gathered recent crashes

I wasn't able to find any other reasonable explanation what is causing the issue other than multiple threads are trying to update the cursor or mCurrentFilter at the same time. I decided to synchronize the methods which are changing mCurrentFilter or the Cursor. I believe it can't hurt us and there is a chance it might fix the issue.

To test:

  1. Open Reader
  2. Search some queries -> eg. "a", "an", "and", "andr" ...
  3. Type "a" and notice all the previous searches "a", "an", "and", "andr" are suggested to you
  4. Type "android" and make sure the search works as expected.

Update release notes:

  • The change is too minor

Copy link
Contributor

@mkevins mkevins left a comment

Choose a reason for hiding this comment

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

This looks like a good approach to me. I didn't do an exhaustive search through the code paths of these methods. Do you know whether the calls will generally be non-blocking? I'm trying to rule out chances for dead-lock (but haven't diagrammed the control flow graph). If it's more-or-less linear, I think this is definitely the way to go. 👍

@malinajirka
Copy link
Contributor Author

Thanks @mkevins!

I'm trying to rule out chances for dead-lock

I think there is only one resource so I believe it's dead-lock free.

@mkevins mkevins self-requested a review August 20, 2019 14:12
Copy link
Contributor

@mkevins mkevins left a comment

Choose a reason for hiding this comment

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

I think there is only one resource so I believe it's dead-lock free.

Sounds good then. Nice work @malinajirka 🎉

@malinajirka malinajirka merged commit a1b5e47 into develop Aug 20, 2019
@malinajirka malinajirka deleted the issue/9987-reader-search branch August 20, 2019 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants