-
Notifications
You must be signed in to change notification settings - Fork 156
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
issue #2510 - make FHIRPathUtil threadsafe #2514
Conversation
Is this a nominal change? e.g. does not impact the benchmark? or is this additive? I'm expecting no impact, but want to confirm in discussion. |
Yes, the blast radius is pretty small on this one. We just avoid using a shared FHIRPathEvaluator. There is a potential for this to slow things down just a bit, but constructing FHIRPathEvaluators should be relatively cheap. If it ends up needing optimization, it would be possible to introduce a ThreadLocal cache, but I don't think it will even register on a profiler. |
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>
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.
LGTM
@@ -88,8 +88,6 @@ | |||
import com.ibm.fhir.path.exception.FHIRPathException; | |||
|
|||
public final class FHIRPathUtil { | |||
private static FHIRPathEvaluator evaluator = FHIRPathEvaluator.evaluator(); |
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.
this shared FHIRPathEvaluator was the real culprit
Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
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