TRUNK-4253:Made PatientService.savePatient threadsafe #2615

samuelmale · 2018-03-07T11:59:58Z

Ticket: https://issues.openmrs.org/browse/TRUNK-4253

suthagar23 · 2018-03-07T17:56:29Z

api/src/test/java/org/openmrs/api/PatientServiceTest.java

+		t1.start();
+		t2.start();
+
+		assertTrue(isSavePatientThreadsafe.get());


You should check for assertFalse since the default value is set to false.

suthagar23 · 2018-03-07T18:04:00Z

api/src/test/java/org/openmrs/api/PatientServiceTest.java

@@ -1848,6 +1853,48 @@ public void savePatient_shouldUpdateAnExistingPatient() throws Exception {
 		assertTrue("The gender should be new", patient2.getGender().equals("F"));
 	}

+	@Test
+	public void savePatient_shouldFailSaveIfPatientsWithSameIdentifierAreSaved() {
+	    isSavePatientThreadsafe = new AtomicBoolean(false);


Just one opinion,
isSavePatientThreadsafe would be changed to true by default or isSavePatientThreadUnSafe = false by default will good for the naming. Because at the beginning all threads are in safe state and it will become the critical stage while you are performing the saving method.

@samuelmale did you see the above?

Cool, I have just seen its significancy @dkayiwa

I'm sorry

coveralls · 2018-03-07T23:33:30Z

Coverage increased (+0.04%) to 59.119% when pulling aca88d8 on samuelmale:TRUNK-4253 into 33f2195 on openmrs:master.

dkayiwa · 2018-03-27T23:40:16Z

@samuelmale does the unit test fail when you remove the changes in PatientServiceImpl?

samuelmale · 2018-03-28T03:55:36Z

Yes. The test fails @dkayiwa

wluyima · 2018-04-16T19:16:45Z

api/src/test/java/org/openmrs/api/PatientServiceTest.java

@@ -1881,7 +1934,7 @@ public void getAllPatients_shouldFetchVoidedPatientsWhenGivenIncludeVoidedIsTrue
 		List<Patient> allPatients = patientService.getAllPatients(true);
 		// there are 1 voided and 4 nonvoided patients in
 		// standardTestDataset.xml
-		assertEquals(6, allPatients.size());
+		assertEquals(7, allPatients.size());


Why is this necessary?

Hi,
I have been facing this challenge for a while. Its all about Threads saving the data to the "in-memory-database" and effects aren't reverted as the default behavior is to other tests. Could I be missing something when using Threads?
The above happens because of this line. Meaning the patient is saved the effect ain't reverted hence I had to change the number of patients from 6 to 7. Its exactly the same issue in this thread. Could you please advise me on this?

@dkayiwa also asked me about it here.

All I know is that changes in one thread aren't visible in others in tests since the transactions get rollback.

What I don't understand is why it's happening now and not before because the test was passing before your changes

samuelmale · 2018-05-13T21:10:44Z

@wluyima I have added some changes

dkayiwa · 2018-07-11T12:22:40Z

api/src/test/java/org/openmrs/api/PatientServiceTest.java

+
+		try {
+			t1.join();
+			//t2.join();


Did you take a look at this? https://wiki.openmrs.org/display/docs/Java+Conventions#JavaConventions-Donotcommentoutcode

samuelmale · 2018-07-11T14:24:52Z

Just removed the comment @dkayiwa

dkayiwa · 2018-07-13T11:18:46Z

api/src/main/java/org/openmrs/api/impl/PatientServiceImpl.java

 	@Override
 	public Patient savePatient(Patient patient) throws APIException {
+		synchronized(this) {
 		requireAppropriatePatientModificationPrivilege(patient);


Did you intentionally decide not to TAB the code within the synchronised phrase?

dkayiwa · 2018-07-13T11:19:21Z

api/src/test/java/org/openmrs/api/PatientServiceTest.java

+	}
+	public void savePatientOnThread(UserContext ctx, Patient patient) {
+		Context.setUserContext(ctx);
+        Context.openSessionWithCurrentUser();


Can you also fix the above indention? Same with a couple of other places in your commit.

I wonder, but locally, the indention is fine. Just to prove, hit view full file and check.

I have highlighted the it here https://github.com/samuelmale/openmrs-core/blob/fd2aed2031b647f046562da4142851669c3c4061/api/src/test/java/org/openmrs/api/PatientServiceTest.java#L1890-L1894

If this is the unaddressed comment, I'm pretty sure my indention is fine. Have you looked at it as a file. In otherwise have looked at the above link? @dkayiwa

Still your PR doesn't have the proper indentation. See here.
Sometimes it happened to me in the IDE also, but can't figure out using that same IDE at the same time. Better to open this file with another text editor(Sublime or VSCode) to check for the indentations. You can easily find out your issues.

dkayiwa · 2018-07-16T09:36:38Z

api/src/main/java/org/openmrs/api/impl/PatientServiceImpl.java

-		setPreferredPatientIdentifier(patient);
-		setPreferredPatientName(patient);
-		setPreferredPatientAddress(patient);
+		synchronized(this) {


Do you have a reason for using a synchronised block instead of method?

samuelmale · 2018-07-16T12:13:48Z

I'm of the view that we use synchronized blocks, a sync'd method makes it not thread safe @dkayiwa

check the CI failure

dkayiwa · 2018-07-16T12:22:09Z

Can you share the link where you read that?

samuelmale · 2018-07-16T12:55:10Z

I have just run 'mvn clean install' on this branch twice and it builds. I wonder whats happening with CI

@dkayiwa

dkayiwa · 2018-07-17T10:34:15Z

I have restarted the build

samuelmale · 2018-07-17T10:50:36Z

Cool, just waiting for yr review @dkayiwa

samuelmale · 2018-07-17T22:01:33Z

Could you restart the CI build once more @dkayiwa ?

dkayiwa · 2018-07-17T22:38:29Z

Even compiling it locally on your machine should fail too.

samuelmale · 2018-07-17T22:47:41Z

Noo, it actually passes @kayiwa Daniel <danielkayiwa@gmail.com>

…

On Wed, Jul 18, 2018 at 1:38 AM dkayiwa ***@***.***> wrote: Even compiling it locally on your machine should fail too. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2615 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AY4E5bE8x5RqQKvS_iXONlPSNO8fcPn2ks5uHmdpgaJpZM4SgU34> .

dkayiwa · 2018-07-18T13:06:08Z

@samuelmale i have restarted it several times and noticed that some times it fails while other times it passed. Whenever it fails, the error message is as below:

Results :
Failed tests:
PatientServiceTest.getAllPatients_shouldFetchVoidedPatientsWhenGivenIncludeVoidedIsTrue:1938 expected:<6> but was:<7>
PatientServiceTest.savePatient_shouldFailSaveIfPatientsWithSameIdentifierAreSaved:1884 IdentifierNotUniqueException not thrown, method not Threadsafe
Tests run: 4022, Failures: 2, Errors: 0, Skipped: 36

dkayiwa · 2018-07-18T14:19:33Z

I just run it another time and got this:

Results :
Failed tests:
PatientServiceTest.getAllPatients_shouldFetchVoidedPatientsWhenGivenIncludeVoidedIsTrue:1938 expected:<6> but was:<8>
PatientServiceTest.savePatient_shouldFailSaveIfPatientsWithSameIdentifierAreSaved:1884 IdentifierNotUniqueException not thrown, method not Threadsafe
Tests run: 4022, Failures: 2, Errors: 0, Skipped: 36

samuelmale · 2018-07-18T16:35:28Z

Do you mind fetching this branch and testing it out locally?

…

On Wed, Jul 18, 2018, 5:19 PM dkayiwa ***@***.***> wrote: I just run it another time and got this: Results : Failed tests: PatientServiceTest.getAllPatients_shouldFetchVoidedPatientsWhenGivenIncludeVoidedIsTrue:1938 expected:<6> but was:<8> PatientServiceTest.savePatient_shouldFailSaveIfPatientsWithSameIdentifierAreSaved:1884 IdentifierNotUniqueException not thrown, method not Threadsafe — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2615 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AY4E5cQIRUpVdQt283xYplvSS6k_mt9aks5uH0P6gaJpZM4SgU34> .

samuelmale · 2018-07-18T18:39:25Z

But now it builds successfully @dkayiwa

Could you take a second look?

dkayiwa · 2018-07-18T19:05:52Z

Like i said, it is random. I have just rerun it and now you can see the failure.

samuelmale · 2018-07-18T19:23:46Z

So whats really going on, but locally its not random @dkayiwa

dkayiwa · 2018-07-18T19:31:43Z

Tru dat!
Looks like it depends on the computer processing power.

samuelmale · 2018-07-20T10:54:10Z

So whats up @dkayiwa

dkayiwa · 2018-07-22T14:43:29Z

@samuelmale the very fact that the test randomly fails in some instances, makes me feel like it needs some more work.

samuelmale · 2018-07-23T05:23:08Z

This weird behaviour started when I changed from a blocked synchronized block to a method synchronized block @dkayiwa . Do we have reasons as to why we can't use the block synchronization?

dkayiwa · 2018-07-23T06:56:31Z

I remember as if it still failed even with block synchronisation. Any way, you can change it back and will show you by simply running travis again.

samuelmale · 2018-07-23T07:05:06Z

Hahaha okie @dkayiwa

samuelmale · 2018-07-23T08:08:02Z

Whats going on here @dkayiwa .
I has failed again. Could you restart travis?

dkayiwa · 2018-07-23T09:24:55Z

@samuelmale i have just rebuilt it three times. For the first two times, it succeeded. But failed on the third one.

samuelmale · 2018-07-23T09:35:24Z

ahhhhh!!!! But have you ever hit upon such a case? @dkayiwa

dkayiwa · 2018-07-23T14:27:31Z

Ummmmm. Rarely!

suthagar23 · 2018-08-21T00:37:31Z

@samuelmale any more ideas on the above Travis failures?

bejelith · 2021-11-03T05:25:36Z

I think ur problem is that savePatient is not thread safe and you have no idea if ur executing inside a pre-existing transaction. Create a separate hibernate transaction/session and that should work, i also have the habit for explicitly begin/commit every transaction even if single statement.

suthagar23 reviewed Mar 7, 2018

View reviewed changes

samuelmale force-pushed the TRUNK-4253 branch from 061af80 to ce0953e Compare March 7, 2018 23:12

wluyima reviewed Apr 16, 2018

View reviewed changes

samuelmale force-pushed the TRUNK-4253 branch from ce0953e to 816e6c4 Compare May 13, 2018 21:09

dkayiwa reviewed Jul 11, 2018

View reviewed changes

samuelmale force-pushed the TRUNK-4253 branch from 816e6c4 to fd2aed2 Compare July 11, 2018 14:24

dkayiwa reviewed Jul 13, 2018

View reviewed changes

samuelmale force-pushed the TRUNK-4253 branch from fd2aed2 to 534f6e3 Compare July 16, 2018 09:34

dkayiwa reviewed Jul 16, 2018

View reviewed changes

samuelmale force-pushed the TRUNK-4253 branch from 534f6e3 to e32fd57 Compare July 16, 2018 09:56

samuelmale force-pushed the TRUNK-4253 branch from e32fd57 to 92a1880 Compare July 17, 2018 20:13

TRUNK-4253:Made PatientService.savePatient threadsafe

aca88d8

samuelmale force-pushed the TRUNK-4253 branch from 92a1880 to aca88d8 Compare July 23, 2018 07:48

dkayiwa force-pushed the master branch from d7b09f1 to 567d861 Compare August 2, 2018 15:36

samuelmale closed this Sep 6, 2018

TRUNK-4253:Made PatientService.savePatient threadsafe #2615

TRUNK-4253:Made PatientService.savePatient threadsafe #2615

Conversation

samuelmale commented Mar 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 7, 2018 • edited Loading

dkayiwa commented Mar 27, 2018

samuelmale commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelmale commented May 13, 2018

Choose a reason for hiding this comment

samuelmale commented Jul 11, 2018

Choose a reason for hiding this comment

dkayiwa Jul 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelmale commented Jul 16, 2018

dkayiwa commented Jul 16, 2018

samuelmale commented Jul 16, 2018

dkayiwa commented Jul 17, 2018

samuelmale commented Jul 17, 2018

samuelmale commented Jul 17, 2018

dkayiwa commented Jul 17, 2018

samuelmale commented Jul 17, 2018 via email

dkayiwa commented Jul 18, 2018

dkayiwa commented Jul 18, 2018 • edited Loading

samuelmale commented Jul 18, 2018 via email

samuelmale commented Jul 18, 2018

dkayiwa commented Jul 18, 2018

samuelmale commented Jul 18, 2018

dkayiwa commented Jul 18, 2018 • edited Loading

samuelmale commented Jul 20, 2018

dkayiwa commented Jul 22, 2018

samuelmale commented Jul 23, 2018

dkayiwa commented Jul 23, 2018

samuelmale commented Jul 23, 2018

samuelmale commented Jul 23, 2018

dkayiwa commented Jul 23, 2018

samuelmale commented Jul 23, 2018

dkayiwa commented Jul 23, 2018

suthagar23 commented Aug 21, 2018

bejelith commented Nov 3, 2021 • edited Loading

coveralls commented Mar 7, 2018 •

edited

Loading

dkayiwa Jul 13, 2018 •

edited

Loading

dkayiwa commented Jul 18, 2018 •

edited

Loading

dkayiwa commented Jul 18, 2018 •

edited

Loading

bejelith commented Nov 3, 2021 •

edited

Loading