cbt icon indicating copy to clipboard operation
cbt copied to clipboard

review and correct thread local storage across nailgun threads

Open cvogt opened this issue 8 years ago • 3 comments

e.g. in/out/err, etc related http://stevenskelton.ca/threadlocal-variables-scala-futures/

cvogt avatar Mar 23 '17 14:03 cvogt

I noticed that the cbt sources use java.nio.file.Path quite a bit. I had a lot of problems with relative paths when adding nailgun support for scalafmt. I found it useful to create my own AbsolutePath wrapper to avoid relative paths that implicitly assume sys.props("user.dir") is correct. I put everything into a Context

in: InputStream
err: PrintStream
out: PrintStream
workingDir: AbsolutePath // or wd or pwd

that I passed around.

olafurpg avatar Mar 23 '17 14:03 olafurpg

yep, tons of Path and File in CBT. Mostly to make it easy to use JDK functions. Was planning on moving everything to Path. How do you guarantee the path is absolute in your wrapper?

I haven't had problems with relative paths, as you usually start with an absolute one CBT handed you, e.g. projectDirectory / "foo" / "bar" (sometimes with a / ".." in there, which could mean trouble just like symlinks, but hasn't much yet).

cvogt avatar Mar 24 '17 04:03 cvogt

Scenario where this could matter, user1 writes a build with no projectDirectory prefix (e.g., Paths.get("build", "deps.conf")) and everything works because the cbt nailgun server happened to launch in the same directory as the build. user2 clones the repo and the build fails because the cbt nailgun server launched in another project. Even worse, there's no crash because "build/deps.conf" references the wrong configuration.

Ammonite ops has rich data structures for handling absolute vs. relative paths (http://www.lihaoyi.com/Ammonite/#Ammonite-Ops). For example, relative path constructors take an implicit working directory so you have to at least consciously import the working directory, which could as simple as import ctx._ from the build context. Other nice safety-guards is that it's impossible to concatenate two absolute paths or append an absolute path to a relative path.

Relying on sys.props(user.dir) is a recipe for unexpected runtime errors IMO. Documenting absolute vs. relative paths in the type system is a fairly cheap way to avoid these errors.

olafurpg avatar Mar 24 '17 08:03 olafurpg