Make source map cache in StackTraceDeobfuscator thread-safe#10361
Make source map cache in StackTraceDeobfuscator thread-safe#10361Samin061 wants to merge 2 commits into
Conversation
niloc132
left a comment
There was a problem hiding this comment.
Unless I'm mistaken, this is still slightly racy when it is accessed - the get/put calls should be replaced with computeIfAbsent so that we only write once? Probably not a dangerous race, but in the same cases where the old code could lead to a race/deadlock, we could end up doing the code twice. If you're cleaning this up, let's clean it all?
Related, but not necessarily blocking: I think SymbolCache could be more efficient too, at least not requiring exclusive readers, only writing should block reading (and writes are only possible for the first read, likewise a computeIfAbsent).
|
|
||
| private final Map<String, SourceMapping> sourceMaps = new HashMap<String, SourceMapping>(); | ||
| private final Map<String, SourceMapping> sourceMaps = | ||
| new ConcurrentHashMap<String, SourceMapping>(); |
There was a problem hiding this comment.
Generic args need not be explicitly specified
| new ConcurrentHashMap<String, SourceMapping>(); | |
| private final Map<String, SourceMapping> sourceMaps = new ConcurrentHashMap<>(); |
|
Good point. Switched loadSourceMap to computeIfAbsent so the parse only runs once per key, and dropped the explicit generic args per your inline note. I left SymbolCache alone to keep this focused on the sourceMaps race. The read/write split there is a bit more involved since the inner maps currently rely on the synchronized block, so I'd rather do it as a separate change if that's alright. |
StackTraceDeobfuscator caches per-permutation source maps in a plain HashMap, and a single shared instance reads and writes it from every request thread (RemoteLoggingServiceImpl and RequestFactoryServlet each hold one). Concurrent deobfuscation of sourcemap-capable client stack traces then races on loadSourceMap's unsynchronized get/put, which can drop cache entries, hand back a mapping stored under a different key, or spin on a corrupted table. The sibling symbol cache in this class is already synchronized; sourceMaps was overlooked. Switch it to ConcurrentHashMap.