Make secured js cache replaceable #59
Conversation
| @Override | ||
| public String getOrCreate(String js, boolean allowNoBraces, Supplier<String> producer) { | ||
| assertConfiguration(allowNoBraces); | ||
| String result = map.get(js); |
There was a problem hiding this comment.
Is this thread-safe here? Maybe it needs to be synchronized so that get() and put() are always executed together?
There was a problem hiding this comment.
This class is supposed to be a reflection of the status quo, and as the javadoc says, is not intended to be thread-safe.
Why?
I'm working with the assumption that the sandbox itself is not thread-safe, and because currently, the cache is per-sandbox, the new default cache, which would also be per-sandbox, does not need to be thread-safe either. As this class is package private, it should not be possible for the user to re-use instances of it across different sandbox instances.
Maybe I am wrong about that initial assumption?
Also, I'd like to state again, that this pull request contains no actual implementation of a concurrent cache, only the interface to have the user put one in.
|
This is an awesome idea! Thank you! I've already started looking through the code and left one comment there so far! |
|
(In case you were not notified of the code comment: I answered. Any other feedback?) |
|
Merged and will be released with version 0.1.15. Thank you for the contribution and please excuse the delay in merging it! |
The secured js cache is currently only available per NashornSandbox instance. I have many Sandbox instances, all executing the same script in separate threads, which is effectively beautified on every request. This makes processing very slow for me.
In this pull request, I abstracted out an interface for that cache, so that I can replace it with my own global & thread-safe implementation. However, as concurrent caching is nontrivial and requires additional dependencies, I chose not to include my own concrete implementation of that cache.