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

Concurrency issue during apply FHIRPathPatchReplace #2510

Closed
seltsamD opened this issue Jun 15, 2021 · 3 comments
Closed

Concurrency issue during apply FHIRPathPatchReplace #2510

seltsamD opened this issue Jun 15, 2021 · 3 comments
Assignees
Labels
bug Something isn't working P1 Priority 1 - Must Have

Comments

@seltsamD
Copy link

Describe the bug
Under concurrent request apply FHIRPathPatch (replace command) produce com.ibm.fhir.model.patch.exception.FHIRPatchException

Environment
IBM FHIR 4.4.0

To Reproduce
Steps to reproduce:

Generate concurrent requests (more than 50 requests per second)
1. Fill valid com.ibm.fhir.model.resource.Parameters resource.
2. Create FHIRPathPatch object
FHIRPathPatch patch = FHIRPathPatch.from(parameters);
3. Apply patch to any resource
patch.apply(...) method throw

Part of stacktrace:	
	com.ibm.fhir.model.patch.exception.FHIRPatchException: Error executing fhirPath
	at com.ibm.fhir.path.patch.FHIRPathPatchReplace.apply(FHIRPathPatchReplace.java:35) ~[fhir-path-4.4.0.jar:4.4.0]
	at com.ibm.fhir.path.patch.FHIRPathPatch.apply(FHIRPathPatch.java:33) ~[fhir-path-4.4.0.jar:4.4.0]
	
	In FHIRPathEvaluator.evaluate(...) method on step contextStack.pop() will throw java.util.EmptyStackException.

Expected behavior
FHIRPathPatch.apply method doesn't throw FHIRPatchException under load.

@seltsamD seltsamD added the bug Something isn't working label Jun 15, 2021
@prb112
Copy link
Contributor

prb112 commented Jun 15, 2021

HI @seltsamD

We've discussed internally. The underlying visitor is not ThreadSafe. A reproducing test case would help, and we can determine if there is any update we can make from there, or recommendation on how to approach this case.

Thank you

Paul

lmsurpre added a commit that referenced this issue Jun 15, 2021
FHIRPathUtil was using a shared static FHIRPathEvaluator but this is not
thread-safe. Now we construct a new evaluator for each request.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Jun 15, 2021
FHIRPathUtil was using a shared static FHIRPathEvaluator but this is not
thread-safe. Now we construct a new evaluator for each request.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
lmsurpre added a commit that referenced this issue Jun 15, 2021
FHIRPathUtil was using a shared static FHIRPathEvaluator but this is not
thread-safe. Now we construct a new evaluator for each request.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@lmsurpre
Copy link
Member

lmsurpre commented Jun 15, 2021

I didn't reproduce this exact issue, but I did find that our FHIRPathUtil class was using a shared a FHIRPathEvaluator where it shouldn't be.

In my test, it manifested like this:

java.util.concurrent.ExecutionException: com.ibm.fhir.path.exception.FHIRPathException: java.util.EmptyStackException  [probeId=7f-0-0-1-c61744c3-60c9-4896-8045-9b4b15e595f1]
	at java.util.concurrent.FutureTask.report(FutureTask.java:122)
	at java.util.concurrent.FutureTask.get(FutureTask.java:192)
	at com.ibm.fhir.path.patch.test.FHIRPathUtilTest.testConcurrentPatches(FHIRPathUtilTest.java:70)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.testng.internal.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:132)
	at org.testng.internal.TestInvoker.invokeMethod(TestInvoker.java:599)
	at org.testng.internal.TestInvoker.invokeTestMethod(TestInvoker.java:174)
	at org.testng.internal.MethodRunner.runInSequence(MethodRunner.java:46)
	at org.testng.internal.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:822)
	at org.testng.internal.TestInvoker.invokeTestMethods(TestInvoker.java:147)
	at org.testng.internal.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146)
	at org.testng.internal.TestMethodWorker.run(TestMethodWorker.java:128)
	at java.util.ArrayList.forEach(ArrayList.java:1257)
	at org.testng.TestRunner.privateRun(TestRunner.java:764)
	at org.testng.TestRunner.run(TestRunner.java:585)
	at org.testng.SuiteRunner.runTest(SuiteRunner.java:384)
	at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:378)
	at org.testng.SuiteRunner.privateRun(SuiteRunner.java:337)
	at org.testng.SuiteRunner.run(SuiteRunner.java:286)
	at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:53)
	at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:96)
	at org.testng.TestNG.runSuitesSequentially(TestNG.java:1218)
	at org.testng.TestNG.runSuitesLocally(TestNG.java:1140)
	at org.testng.TestNG.runSuites(TestNG.java:1069)
	at org.testng.TestNG.run(TestNG.java:1037)
	at org.testng.remote.AbstractRemoteTestNG.run(AbstractRemoteTestNG.java:115)
	at org.testng.remote.RemoteTestNG.initAndRun(RemoteTestNG.java:251)
	at org.testng.remote.RemoteTestNG.main(RemoteTestNG.java:77)
Caused by: com.ibm.fhir.path.exception.FHIRPathException: java.util.EmptyStackException  [probeId=7f-0-0-1-c61744c3-60c9-4896-8045-9b4b15e595f1]
	at com.ibm.fhir.path.evaluator.FHIRPathEvaluator.evaluate(FHIRPathEvaluator.java:256)
	at com.ibm.fhir.path.evaluator.FHIRPathEvaluator.evaluate(FHIRPathEvaluator.java:229)
	at com.ibm.fhir.path.evaluator.FHIRPathEvaluator.evaluate(FHIRPathEvaluator.java:209)
	at com.ibm.fhir.path.evaluator.FHIRPathEvaluator.evaluate(FHIRPathEvaluator.java:173)
	at com.ibm.fhir.path.evaluator.FHIRPathEvaluator.evaluate(FHIRPathEvaluator.java:149)
	at com.ibm.fhir.path.util.FHIRPathUtil.evaluateToSingle(FHIRPathUtil.java:977)
	at com.ibm.fhir.path.util.FHIRPathUtil.add(FHIRPathUtil.java:896)
	at com.ibm.fhir.path.patch.test.FHIRPathUtilTest$Add.call(FHIRPathUtilTest.java:136)
	at com.ibm.fhir.path.patch.test.FHIRPathUtilTest$Add.call(FHIRPathUtilTest.java:1)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)
Caused by: java.util.EmptyStackException
	at java.util.Stack.peek(Stack.java:102)
	at java.util.Stack.pop(Stack.java:84)
	at com.ibm.fhir.path.evaluator.FHIRPathEvaluator$EvaluatingVisitor.evaluate(FHIRPathEvaluator.java:302)
	at com.ibm.fhir.path.evaluator.FHIRPathEvaluator$EvaluatingVisitor.access$43(FHIRPathEvaluator.java:297)
	at com.ibm.fhir.path.evaluator.FHIRPathEvaluator.evaluate(FHIRPathEvaluator.java:254)
	... 12 more

I added my unit test and the fix for this to #2514.
If you would like to help verify it, please check out my branch (or main once its merged). Otherwise, this will be in the next release of the IBM FHIR Server.

@lmsurpre lmsurpre self-assigned this Jun 15, 2021
@lmsurpre lmsurpre added the P1 Priority 1 - Must Have label Jun 15, 2021
lmsurpre added a commit that referenced this issue Jun 15, 2021
Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
@prb112
Copy link
Contributor

prb112 commented Jul 28, 2021

HI @seltsamD this issue has an associated PR that should address this issue. We've tested it in our environments, and are closing the issue. Should this not be satisfactory please let us know, and we can work with you on resolving any remaining elements to your issue, thank you, Paul

@prb112 prb112 closed this as completed Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working P1 Priority 1 - Must Have
Projects
None yet
Development

No branches or pull requests

3 participants