Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Fix occasional failed imports due to race conditions
Fixes kaitai-io/kaitai_struct#951 Until now, KSC had occasional problems with resolving imported top-level types, reporting `unable to find type '<imported>'` errors from time to time (see kaitai-io/kaitai_struct#951 for more details and reproduction instructions). It turned out that the importing code ran on multiple threads that were concurrently modifying/reading shared non-thread-safe `HashMap`s without any synchronization, which resulted in race conditions. I thought it would be best to eliminate the race conditions by switching to some thread-safe counterpart to `mutable.HashMap`. After some googling, it turned out that there are two viable options, [`scala.collection.concurrent.TrieMap`](https://www.scala-lang.org/api/2.13.13/scala/collection/concurrent/TrieMap.html) and [`java.util.concurrent.ConcurrentHashMap`](https://docs.oracle.com/javase/8/docs/api/java/util/concurrent/ConcurrentHashMap.html). However, `TrieMap` doesn't seem to be implemented for Scala.js (scala-js/scala-js#4866) and it is `final`, so we cannot use it as a base class of `ClassSpecs` instead of `mutable.HashMap` that we're using now. `ConcurrentHashMap` (despite being in the `java.util.concurrent.*` package, suggesting that it would only be available on JVM) surprisingly appears to be available in Scala.js (scala-js/scala-js#1487), so in theory we could use it even in `shared/src/...` code, but I haven't figured out how to make `ClassSpecs` extend from it. It must be added that since Scala 2.13, the inheritance from `HashMap` was deprecated, so we'll have to find another way, but for now it works. The unsynchronized hash map accesses in the `JavaClassSpecs.cached()` are in JVM-specific code (`jvm/src/...` folder), so I've just changed the private `relFiles` and `absFiles` fields to use `ConcurrentHashMap`, thus resolving the race conditions here. (`TrieMap` would work too - there's probably no practical difference for our use case.) For the `LoadImports.loadImport()` method, I've just wrapped the accesses to shared `ClassSpecs` in a manual `synchronized` block. According to some on the internet, using `synchronized` is kind of discouraged due to being error-prone in favor of using existing thread-safe types or immutable types (Scala generally seems to prefer immutable types, but I can't imagine how they could be used in this case), but I believe it's the easiest way to solve the problem here.
- Loading branch information