Github user jbertram commented on a diff in the pull request:
https://github.com/apache/activemq-artemis/pull/2228#discussion_r208667762
--- Diff:
artemis-selector/src/main/java/org/apache/activemq/artemis/selector/impl/SelectorParser.java
---
@@ -80,11 +78,15 @@ public static BooleanExpression parse(String sql)
throws FilterException {
StrictParser parser = new StrictParser(new
StringReader(actual));
e = parser.JmsSelector();
}
- cache.put(sql, e);
+ synchronized (cache) {
--- End diff --
I imagine there might be a "better" option here in terms of performance
than using `synchronized`, but then again this isn't exactly on the hot path.
This code is only executed when a selector/filter is first created (e.g. for a
JMS consumer, for a bridge, etc.), and it is only executed once (i.e. execution
isn't looped). Most situations where cache growth is problematic (e.g. lots of
consumers with lots of different selectors connecting and disconnecting often)
are generally considered messaging anti-patterns anyway. For such pathological
cases using `synchronized` is potentially quite a bit faster than a complete
lack of thread safety because it prevents unfettered cache growth. In my
opinion @franz1981's time is better spent elsewhere as this smacks of premature
optimization.
---
