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

Fixed: Xapian crash scenarios. #3885

Merged
merged 8 commits into from
Jun 20, 2024
Merged

Fixed: Xapian crash scenarios. #3885

merged 8 commits into from
Jun 20, 2024

Conversation

MohitMaliFtechiz
Copy link
Collaborator

@MohitMaliFtechiz MohitMaliFtechiz commented Jun 18, 2024

Fixes #3877

  • There was a concurrency issue in the search functionality in a very specific condition, causing data corruption and race conditions when users searched for different terms simultaneously.
  • Introduced a global Mutex in the SearchResultsWithTerm class to control access to search results.
  • Applied the Mutex in the SearchState class to lock execution and ensure that only one coroutine can process search results at a time. Since here we are getting the data from libzim this global mutex will prevent getting data via multiple coroutines.
  • Enhanced retrieval of suggestion lists from libzim.
  • Implemented task cancellation if the fragment is not visible.
  • Refactored getVisibleResults into a suspend method to ensure efficient job cancellation.
  • Handled exceptions thrown by coroutines.
  • Employed lifecycleScope for coroutine launch to prevent unnecessary calls when the fragment is hidden.
  • Added unit test cases for testing the previous job is canceled properly.
  • Testing the render method with different scenarios to ensure that libzim does not crash due to a broken call stack.

* Enhanced retrieval of suggestion lists from libzim.
* Implemented task cancellation if the fragment is not visible.
* Refactored getVisibleResults into a suspend method to ensure efficient job cancellation.
* Handled exceptions thrown by coroutines.
* Transitioned to Kotlin Flow instead of Flowable after making getVisibleResults suspend.
* Employed lifecycleScope for coroutine launch to prevent unnecessary calls when the fragment is hidden.
@MohitMaliFtechiz MohitMaliFtechiz marked this pull request as draft June 18, 2024 15:33
Copy link

codecov bot commented Jun 18, 2024

Codecov Report

Attention: Patch coverage is 76.11940% with 16 lines in your changes missing coverage. Please review.

Project coverage is 53.54%. Comparing base (9f94965) to head (193c0d1).

Files Patch % Lines
...rg/kiwix/kiwixmobile/core/search/SearchFragment.kt 62.50% 4 Missing and 8 partials ⚠️
...x/kiwixmobile/core/search/viewmodel/SearchState.kt 88.88% 1 Missing and 1 partial ⚠️
...wixmobile/core/search/viewmodel/SearchViewModel.kt 85.71% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #3885      +/-   ##
============================================
- Coverage     53.54%   53.54%   -0.01%     
+ Complexity     1402     1396       -6     
============================================
  Files           301      302       +1     
  Lines         11569    11588      +19     
  Branches       1526     1527       +1     
============================================
+ Hits           6195     6205      +10     
- Misses         4376     4381       +5     
- Partials        998     1002       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…s cancelled before requesting suggestion results from libzim.

* Enhanced the loadMoreSearchResults method to accommodate this modification.
* Refined the render method to effectively cancel any previously running tasks upon its subsequent execution.
…concurrency issues.

* Applied mutex locking in the SearchState class to ensure safe access to search results when users search for different terms.
* Testing the render method with different scenarios to ensure that libzim do not crash due to broken call stack.
@MohitMaliFtechiz MohitMaliFtechiz marked this pull request as ready for review June 19, 2024 19:06
@kelson42
Copy link
Collaborator

kelson42 commented Jun 19, 2024

@MohitMaliFtechiz Thank you for the fix and the hard work. Does this PR has any other impact for the users (positive or negative) beside fixing the crash scenario?

@MohitMaliFtechiz Does this PR impacts #2505 as well?

@MohitMaliFtechiz MohitMaliFtechiz marked this pull request as draft June 19, 2024 19:19
@MohitMaliFtechiz MohitMaliFtechiz marked this pull request as ready for review June 19, 2024 19:46
@MohitMaliFtechiz
Copy link
Collaborator Author

MohitMaliFtechiz commented Jun 19, 2024

@MohitMaliFtechiz Thank you for the fix and the hard work. Does this PR has any other impact for the users (positive or negative) beside fixing the crash scenario?

@kelson42 No it will not impact the user, it only fixes the crashing scenario.

@MohitMaliFtechiz Does this PR impacts #2505 as well?

@kelson42 This PR is for suggestions, and we are now fully using the coroutines for searching the suggestions from the zim file. But #2505 is related to FileSearching(to get the zim files from storage that is different from this PR), and for that, we are currently using the Rxjava. So this PR does not impact the #2505.

@kelson42 kelson42 merged commit 99fe898 into main Jun 20, 2024
10 checks passed
@kelson42 kelson42 deleted the Fixes#3877 branch June 20, 2024 04:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Xapian crash scenarios
3 participants