[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[GitHub] activemq-artemis pull request #2228: ARTEMIS-2017 SelectorParser cache not t...

Github user jbertram commented on a diff in the pull request:

    --- 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.