`ScalaSourceFile.createFromPath` is not thread-safe
The actual reason why `ScalaSourceFile.createFromPath` isn't thread-safe is because it relies on a cached HandleFactory instance, which isn't thread-safe. It's so obvious that the current implementation of `ScalaSourceFile.createFromPath` is broken that I'm really surprised we have only found out about this now. I'm wondering now how many other bugs may have been a consequence of this incorrect implementation.
The simple fix is to create a fresh instance of `HandleFactory` for every call. However, this may decrease performances (the reason why `HandleFactory` isn't thread-safe is that it caches data for performance reasons). My current idea for fixing this is to create by default a fresh instance of `HandleFactory`, unless the caller explictly pass one (hence, the caller is responsible of ensuring correctnes, i.e., the passed `HandleFactory` instance is not shared across threads, unless a proper synchronization is in place).
In scala-ide:82f0809e63e8f86ecd26c7bcdd7c190a6f9952ac Made `ScalaSourceFile.createFromPath` implementation thread-safe
The former implementation of `ScalaSourceFile.createFromPath` was not
thread-safe, which was problematic because the method is called concurrently
from different threads. The actual reason for the race-condition is that
`HandleFactory` is not thread-safe. (Side note, this is why I was keeping
getting "Resource XXX does not exist." exceptions while reviewing scala-search
PR #68).
The fix consists in making sure that no `HandleFactory` instance is shared
across threds. One relevant point is that `HandleFactory` performs caching for
speeding up retrieving an `Openable` from a `path` (the caching is the actual
reason why the class isn't thread-safe). This is an important aspect to keep in
mind, and which should motivate the implemented fix.
I could see two possible ways to fix `ScalaSourceFile.createFromPath`:
1) Create a fresh instance each time (and also provide an additional method to
allow callers to explictly pass a `HandleFactory` when they know the instance
isn't shared among threads).
2) Delegate the responsability of never sharing `HandleFactory` instances among
threads to a `ThreadLocal`.
Option 2) seemed to be the best fix, because
- correctness is ensured by design,
- no changes are required from the caller, and
- it maximize opportunities for re-using the cache maintained by `HandleFactory`.
Last but not least, I've taken the liberty of breaking both source and binary
compatibility. I believe the `handleFactory` instance was public for historical
reasons, and I don't expect anyone to complain about this breakage. However, I
could also re-introduce it, making it a `def`, mark it @deprecated, and simply
return a fresh `HandleFactory` instance.
Fixes #1001846
In scala-ide:15fc83b9b59ba7a73ecb371510cd4bafebbb3864 Made `ScalaSourceFile.createFromPath` implementation thread-safe
The former implementation of `ScalaSourceFile.createFromPath` was not
thread-safe, which was problematic because the method is called concurrently
from different threads. The actual reason for the race-condition is that
`HandleFactory` is not thread-safe. (Side note, this is why I was keeping
getting "Resource XXX does not exist." exceptions while reviewing scala-search
PR #68).
The fix consists in making sure that no `HandleFactory` instance is shared
across threds. One relevant point is that `HandleFactory` performs caching for
speeding up retrieving an `Openable` from a `path` (the caching is the actual
reason why the class isn't thread-safe). This is an important aspect to keep in
mind, and which should motivate the implemented fix.
I could see two possible ways to fix `ScalaSourceFile.createFromPath`:
1) Create a fresh instance each time (and also provide an additional method to
allow callers to explictly pass a `HandleFactory` when they know the instance
isn't shared among threads).
2) Delegate the responsability of never sharing `HandleFactory` instances among
threads to a `ThreadLocal`.
Option 2) seemed to be the best fix, because
- correctness is ensured by design,
- no changes are required from the caller, and
- it maximize opportunities for re-using the cache maintained by `HandleFactory`.
Last but not least, the fix is both source and binary compatible. However,
``ScalaSourceFile.handleFactory`` is now deprecated, as I believe it was public for
historical reasons.
Fixes #1001846
(cherry picked from commit 82f0809e63e8f86ecd26c7bcdd7c190a6f9952ac)
Conflicts:
org.scala-ide.sdt.core/src/scala/tools/eclipse/javaelements/ScalaSourceFile.scala