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 issue #924 caused by memory leak #926

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ariadng
Copy link

@ariadng ariadng commented Dec 8, 2022

This pull request provides fix for Issue #924.

The Problem

Method getPreviousContactFacts in PreviousContactRepository class has unhandled recursion that caused the ANC app to crash when it couldn't find previous contact data. For more detailed problem and analysis, please refer to the Issue #924 page.

Solution

Instead of calling a method recursively, this fix provides a solution to limit the previous contact data searching while still answers the requirement of returning the latest referral contact (with negative contact number) or immediate previous contact. That way, memory leak problem that caused the app to crash can be prevented.

What's Changed

To get immediate previous contact, this fix use two methods:

  1. getPreviousContactFacts without checkNegative parameter
  2. getPreviousContactFacts with checkNegative parameter

The first method does the actual search. It will search the contact based on contactNo parameter that can be positive or negative. So it can return both previous contact and referral contact based on the parameter.

The second method is the method that handles the request from other classes. It has the checkNegative parameter to do the referral contact searching when needed. If the provided checkNegative value is true it will call the first method to search previous contacts with contactNo from -10 to -1, before searching for the current contactNo value.

Obviously, the search limit can be changed by modifying the value of negativeContactsLimit on PreviousContactRepository.java file line 421. If you are going to modify the limit, please enter a positive number (number 10 to search referral contacts from -10 to -1.

Because this doesn't change the input, output, and how the method is called, this fix doesn't need any adjustments in other classes.

Notes

  1. I have thought of better algorithm for these methods. But I think the current fix is sufficient for now because I need to make sure of how the app handles contact data when the client finishes her visit and it will take longer time.
  2. This fix has been tested by Indonesian tech team using multiple accounts and many clients data (including the ones that caused the app to crash before) and is working as expected without any problems.

CC: @Naima-Bashir @dubdabasoduba

Copy link
Contributor

@SebaMutuku SebaMutuku left a comment

Choose a reason for hiding this comment

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

Please fix the codacy issues and add tests for the functionality .
Ensure to cover the exception block

@ariadng
Copy link
Author

ariadng commented Dec 9, 2022

Hello @SebaMutuku thank you for the review.

I have updated the code to fix Codacy issues. I have also integrate Codacy to Indonesian local repository for more linear and faster future reviewing process.

I will submit the PR once unit testing is done.

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.

2 participants