PatientService.savePatient() isn't threadsafe and can allow duplicate patient identifiers

Development
1

Hi all
Iā€™m currently working on this ticket (TRUNK-4253). I have certainly done some work and came up with a fix :smile: . But I uncertainly got blocked by something small :slightly_smiling_face:.

What the issue is about

While saving a Patient in PatientService.savePatient(), two/more concurrently executing threads can ably save a Patient with duplicate patient identifiers. This is not a right behavior and should be fixed.
I was able to reproduce this and came up with an ideal solution.

Work arounds on this issue attained

I came up with some fixes in the PatientService.savePatient(). 
@Override
@Transactional(isolation = Isolation.SERIALIZABLE)
public Patient savePatient(Patient patient) throws APIException {
 
	synchronized(this) {
	requireAppropriatePatientModificationPrivilege(patient);

if (!patient.getVoided() && patient.getIdentifiers().size() == 1) {
		patient.getPatientIdentifier().setPreferred(true);
	}
    
	if (!patient.getVoided()) {
		checkPatientIdentifiers(patient);
	}

	setPreferredPatientIdentifier(patient);
	setPreferredPatientName(patient);
	setPreferredPatientAddress(patient);

	return dao.savePatient(patient);
    }
}

And the above works fine. :slight_smile:

What is blocking me

I came up with some unit tests that reproduce the bug, then made the above changes that fix the bug. I came up with a class variable static AtomicBoolean isSavePatientThreadsafe which is false by default and only made true when org.openmrs.api.IdentifierNotUniqueException is caught. Am sure this exception is thrown when the second Thread tries to save a patient of duplicate Identifier. But I wonder why my Assertion fails!

@Test

public void savePatient_shouldFailSaveIfPatientsWithSameIdentifierAreSaved() {
    
isSavePatientThreadsafe = new AtomicBoolean(false);
       
 UserContext ctx = Context.getUserContext();
	
	Patient patient1 = new Patient();
	patient1.setGender("M");
	patient1.addName(new PersonName("Test1", "Sam", "Test"));
	
	Patient patient2 = new Patient();
	patient2.setGender("M");
	patient2.addName(new PersonName("Test2", "Samuel", "Test"));
	
	Runnable r1 = () -> savePatientOnThread(ctx, patient1);
	       
	Runnable r2 = () -> savePatientOnThread(ctx, patient2);
			
	Thread t1 = new Thread(r1);
	Thread t2 = new Thread(r2);
	
	t1.start();
	t2.start();
	
	assertTrue(isSavePatientThreadsafe.get()); //Why does this Assertion fail!
	
}
public void savePatientOnThread(UserContext ctx, Patient patient) {
	Context.setUserContext(ctx);
    Context.openSessionWithCurrentUser();
    authenticate();
	PatientIdentifier patientIdentifier = new PatientIdentifier("103-9", new PatientIdentifierType(1), new Location(1));
	patientIdentifier.setPreferred(true);
	patient.addIdentifier(patientIdentifier);
	try {
	   patientService.savePatient(patient);
	}
	catch(org.openmrs.api.IdentifierNotUniqueException e) {
		isSavePatientThreadsafe.set(true);
	}
    Context.closeSessionWithCurrentUser();
}

You can get details in the PR .
Thanks in advance guys :slight_smile:
CC: @dkayiwa , @mksd , @mogoodrich , @wyclif etc

2

Actually, you have changed the concurrent savePatient() method to sequential savePatient() by adding the synchronized block. So there can be no more duplicate patient identifiers through this implementation.

According to your test method,

You have set the isSavePatientThreadsafe to false at the beginning by default. When there are any duplicate identifiers found in the system while saving the patients, then the variable isSavePatientThreadsafe will be changed as true. ]

But here, There is no way to throw the IdentifierNotUniqueException since the block is synchronized. So the isSavePatientThreadsafe variable will not change (will be false). So when your are checking the isSavePatientThreadsafe through assertTrue(isSavePatientThreadsafe.get()); will give an assertion fail.

I think, If you check with assertFalse(isSavePatientThreadsafe.get());, then it will give the success.

If I missed anything here, pls apologize my answer :smiley:

1 Like
3

Hi @suthagar23
Thanks for looking into this, but you probably need a second glance at it. The question is that:

What throws the "org.openmrs.api.IdentifierNotUniqueException " and why?

This Exception is thrown during validation before saving a Patient to the database. If you look carefully at the above Patient objects, they both have the same Identifiers, actually its the synchronization the helps us to cause this exception to be thrown. How ?
When all threads access the PatientService.savePatient() one will be allowed to execute, saving the patient, then when the remaining thread comes, it will try to save the Patient with an already taken (duplicate) Identifier and this will cause the exception to be thrown. :slight_smile: .
Try to look into this again.
Thanks !
1 Like
4

Just glancing at this. Since you start separate threads in t1 and t2, do you know that they have done their work before your assertion is executed? Would ensuring the threads have executed before asserting help?

2 Likes
5

Thanks very much @burke , you saved alot of my time.
Blocker resolved :smile:

6

Thanks @suthagar23