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

ARTEMIS-2017 SelectorParser cache not thread-safe #2228

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@jbertram
Copy link
Contributor

commented Aug 8, 2018

No description provided.

@@ -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) {

This comment has been minimized.

Copy link
@michaelandrepearce

michaelandrepearce Aug 8, 2018

Contributor

Must be a better way than sync blocks.

@franz1981 ideas?

This comment has been minimized.

Copy link
@jbertram

jbertram Aug 8, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@franz1981

franz1981 Aug 8, 2018

Contributor

@jbertram @michaelandrepearce If it is not an hot path I thing that what Justin is suggesting makes totally sense: anyway if we can afford to put guava in place here I believe that https://google.github.io/guava/releases/16.0/api/docs/com/google/common/cache/CacheBuilder.html#maximumSize(long) would be a good option too.
It will scale with a factor specified by the concurrent level specified, but it could (I do not remember TBH) introduce some behavioural changes from the current impl.
I understand both @michaelandrepearce and @jbertram opinions so I think that the general rule to balance changes vs effectiveness on common case apply here: I leave you decide how much this tradeoff apply here...
I'm honored to be "summoned" 👍

This comment has been minimized.

Copy link
@michaelandrepearce

michaelandrepearce Aug 8, 2018

Contributor

I would have two points here then:

If perf here isn't critical, why even bother then having an LRUCache? Simply removing it would remove any threading issues right.

If it is critical:

Then a more elegant option than sync blocks would look to update LRUCache to extend ConcurrentLinkedHashMap (https://github.com/ben-manes/concurrentlinkedhashmap) or even better full hog replace it entirely and migrate to using something like Caffeine (https://github.com/ben-manes/caffeine)

This comment has been minimized.

Copy link
@jbertram

jbertram Aug 9, 2018

Author Contributor

My guess is that the LRUCache is there mainly because it was used in Apollo and basically the whole selector implementation from Apollo was ported over to the Artemis code-base during the Apache donation process. It is a simple mitigation against excessive parsing, but perhaps it is premature optimization as well. I don't see a big problem with pulling it out. It would simplify the code-base a bit and eliminate the threading problem as well. I certainly don't see any reason to invest resources into a more robust/elegant/higher-performance cache implementation.

This comment has been minimized.

Copy link
@jbertram

jbertram Aug 9, 2018

Author Contributor

FWIW, the LRUCache is also used in org.apache.activemq.artemis.core.protocol.openwire.OpenWireProtocolManager and access is controlled with synchronized blocks.

This comment has been minimized.

Copy link
@michaelandrepearce

michaelandrepearce Aug 9, 2018

Contributor

On that then, i would simply remove the use in the SelectorParser for this particular case.

Re the actual class, if its used in OpenWireProtocolManager then it would suggest the LRUCache is being used beyond its original scope and should move out of its current location into common util, if it is to be re-used in other area's and then if it is to slowly be used in other areas such us the protocol manager, it may warrant some future efforts to harden it, e.g. instead of everyone needing to add sync blocks itself becomes threadsafe, thus avoiding accidental future concurrent access issues, or even spot where a method is being called and put the needed protection in every time its used. (but if removed the usage in this case, this would be out of scope of this particular PR)

@clebertsuconic

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2018

shouldn't you also protected get? I don't think it's thread safe.

@jbertram jbertram force-pushed the jbertram:ARTEMIS-2017 branch from 0df3ee1 to 9ed5c24 Aug 9, 2018

@jbertram

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2018

@clebertsuconic, I updated to protect get() as well.

ARTEMIS-2017 Eliminate LRUCache from SelectorParser
The LRUCache is not thread-safe and it's usage in SelectorParser could
cause growth beyond its configured max size. Instead of modifying the
LRUCache to be thread-safe or synchronizing access to it in
SelectorParser it should just be removed since it's not on a hot path.

@jbertram jbertram force-pushed the jbertram:ARTEMIS-2017 branch from 9ed5c24 to 731aeba Aug 9, 2018

@michaelandrepearce

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2018

Last change LGTM

clebertsuconic added a commit to clebertsuconic/activemq-artemis that referenced this pull request Aug 9, 2018

@asfgit asfgit closed this in d503bbb Aug 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.