-
Notifications
You must be signed in to change notification settings - Fork 76
IDGEN-134: Update to OpenMRS Platform 2.7.x and Support Java 21 #130
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
base: master
Are you sure you want to change the base?
Changes from 1 commit
208d215
842e505
c51c2d3
18484aa
d486b29
a561767
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,8 @@ | |
| import org.openmrs.module.idgen.IdentifierSource; | ||
| import org.openmrs.module.idgen.IdgenBaseTest; | ||
| import org.openmrs.module.idgen.service.IdentifierSourceService; | ||
| import org.springframework.test.annotation.NotTransactional; | ||
| import org.springframework.transaction.annotation.Propagation; | ||
| import org.springframework.transaction.annotation.Transactional; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.HashSet; | ||
|
|
@@ -37,31 +38,30 @@ public void setUp() throws Exception { | |
| } | ||
|
|
||
| @Test | ||
| @NotTransactional | ||
| public void testUnderLoad() throws Exception { | ||
| @Transactional(propagation = Propagation.NEVER) | ||
| public void testUnderLoad() { | ||
|
|
||
| final List<String> generated = new ArrayList<String>(); | ||
|
|
||
| List<Thread> threads = new ArrayList<Thread>(); | ||
| List<Thread> threads = new ArrayList<>(); | ||
| for (int i = 0; i < NUM_THREADS; ++i) { | ||
| Thread thread = new Thread(new Runnable() { | ||
| @Override | ||
| public void run() { | ||
| Context.openSession(); | ||
| Context.authenticate("admin", "test"); | ||
| IdentifierSource source = getService().getIdentifierSource(4); | ||
| try { | ||
| authenticate(); | ||
| sleep(100); | ||
| Thread thread = new Thread(() -> { | ||
| Context.openSession(); | ||
| Context.authenticate("admin", "test"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain why we needed to make all the changes here?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since multiple threads are writing to the same ArrayList at the same time, it’s not thread-safe. That’s why the test started failing randomly after the update. I fixed it by wrapping the add logic in a synchronized block. Not totally sure why this didn’t happen before, though. |
||
| IdentifierSource source = getService().getIdentifierSource(4); | ||
| try { | ||
| authenticate(); | ||
| sleep(100); | ||
| synchronized (generated) { | ||
| generated.addAll(getService().generateIdentifiers(source, 1, "thread")); | ||
| sleep(100); | ||
| } | ||
| catch (Exception e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| finally { | ||
| Context.closeSession(); | ||
| } | ||
| sleep(100); | ||
| } | ||
| catch (Exception e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| finally { | ||
| Context.closeSession(); | ||
| } | ||
| }); | ||
| thread.start(); | ||
|
|
@@ -77,7 +77,7 @@ public void run() { | |
| } | ||
|
|
||
| assertThat(generated.size(), is(NUM_THREADS)); | ||
| assertThat(new HashSet<String>(generated).size(), is(NUM_THREADS)); | ||
| assertThat(new HashSet<>(generated).size(), is(NUM_THREADS)); | ||
| } | ||
|
|
||
| public void sleep(long time) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,11 @@ | |
| <ref bean="mappingJarResources" /> | ||
| </property> | ||
| <!-- default properties must be set in the hibernate.default.properties --> | ||
| <property name="packagesToScan"> | ||
| <list> | ||
| <value>org.openmrs</value> | ||
| </list> | ||
| </property> | ||
| </bean> | ||
|
|
||
| <bean id="idgenTestTimerFactory" class="org.springframework.scheduling.concurrent.ScheduledExecutorFactoryBean"> | ||
|
|
@@ -39,5 +44,9 @@ | |
| </list> | ||
| </property> | ||
| </bean> | ||
|
|
||
|
|
||
| <bean id="LocationBasedSuffixProvider" class="org.openmrs.module.idgen.suffixprovider.LocationBasedSuffixProvider" /> | ||
|
||
| <bean id="LocationBasedPrefixProvider" class="org.openmrs.module.idgen.prefixprovider.LocationBasedPrefixProvider" /> | ||
|
|
||
| </beans> | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this being removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mseaton I'm getting the following error when running this line:
I'm not exactly sure what’s causing this issue. However, I suspect that the proxy call doesn’t see the registered processor because the processors haven’t been populated in that proxy context. We might not have encountered this in older versions because Spring's proxying and transaction handling used to be more permissive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Requires_New transactional annotation is very important (or at least it's goal is) which is to ensure that identifier generation does not result in duplicate identifiers being issued. It should not be removed without understanding it's purpose and ensuring with sufficient testing (which is difficult in our unit test framework) that the goals continue to be met.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @mseaton 100%. We should not be removing annotations just as a way to fix tests. Though admittedly this could be a difficult one to solve.