Skip to content

Commit 6edd90e

Browse files
authored
Fixes to macOS watcher and ensuring that watch is set up before os.watch() returns (#398)
This PR contains multiple fixes. 1. macOS `FSEventsWatcher` was modified to use the new dispatch queue-based API instead of the old, deprecated run-loop based API. This made closing the watcher easier, without any class-level state to maintain, making sure resource release is always done appropriately. 2. Invoking `watcher.close()` is now threadsafe and idempotent. 3. Invoking `os.watch` now requires the directory to exist, as a sentinel file is written to that directory to make sure `os.watch` is actually watching the directory before it returns. An exception is thrown if the directory does not exist. Tested with tests added to `os/watch/test/src/WatchTests.scala`. These tests work fine in current `main` branch as well: ``` [238] + test.os.watch.WatchTests.emptyFolder.singleFileChangeManyTimes 4558ms [238] + test.os.watch.WatchTests.manyFiles.inManyFoldersSmall 2068ms [238] + test.os.watch.WatchTests.manyFiles.inManyFoldersMedium 3369ms [238] + test.os.watch.WatchTests.manyFiles.inManyFoldersLarge 4056ms [238] + test.os.watch.WatchTests.manyFiles.inManyFoldersLargest 15ms [238] + test.os.watch.WatchTests.manyFiles.inManyFoldersThreaded 28817ms [238] + test.os.watch.WatchTests.manyFiles.inManyFoldersThreadedSequential 26746ms [238] + test.os.watch.WatchTests.openClose.once 15ms ``` These tests reliably exhibit the [JVM crash](com-lihaoyi/mill#5225) in `main` branch: ``` arturaz@Arturass-MacBook-Air os-lib % ./mill -w 'os.watch.jvm[3.3.5].test.test' test.os.watch.WatchTests.emptyFolder.doesNotLeaveSentinel [238/238] os.watch.jvm[3.3.5].test.test [238] --- Running Tests test.os.watch.WatchTests.emptyFolder.doesNotLeaveSentinel --- [238] # [238] # A fatal error has been detected by the Java Runtime Environment: [238] # [238] # SIGSEGV (0xb) at pc=0x000000018409957c, pid=41664, tid=3843 [238] # [238] # JRE version: OpenJDK Runtime Environment Temurin-21.0.1+12 (21.0.1+12) (build 21.0.1+12-LTS) [238] # Java VM: OpenJDK 64-Bit Server VM Temurin-21.0.1+12 (21.0.1+12-LTS, mixed mode, tiered, compressed oops, compressed class ptrs, g1 gc, bsd-aarch64) [238] # Problematic frame: [238] # C [CoreFoundation+0x15057c] __CFCheckCFInfoPACSignature+0x4 [238] # [238] # No core dump will be written. Core dumps have been disabled. To enable core dumping, try "ulimit -c unlimited" before starting Java again [238] # [238] # An error report file with more information is saved as: [238] # /Users/arturaz/work/os-lib/out/os/watch/jvm/3.3.5/test/test.dest/sandbox/hs_err_pid41664.log [238] # [238] # If you would like to submit a bug report, please visit: [238] # https://github.com/adoptium/adoptium-support/issues [238] # The crash happened outside the Java Virtual Machine in native code. [238] # See problematic frame for where to report the bug. [238] # ``` These tests either fail in `main` branch because the behaviour there is different: ``` [238] ----------- Running Tests test.os.watch.WatchTests.nonExistentFolder ----------- [238] X test.os.watch.WatchTests.nonExistentFolder 213ms [238] utest.AssertionError: _root_.os.watch.watch(Seq(wd / "does-not-exist"), onEvent = _ => ()) [238] wd: os.Path = /Users/arturaz/work/os-lib/out/os/watch/jvm/3.3.5/test/test.dest/sandbox/out/scratch/watch/WatchTests/tests/nonExistentFolder [238] utest.asserts.Util$.assertError(Util.scala:20) [238] utest.asserts.Asserts$.interceptImpl(Asserts.scala:49) [238] test.os.watch.WatchTests$.$init$$$anonfun$1$$anonfun$1$$anonfun$1(WatchTests.scala:60) [238] test.os.TestUtil$.prep$$anonfun$1(TestUtil.scala:92) [238] test.os.TestUtil$.mkDir(TestUtil.scala:64) [238] test.os.TestUtil$.prep(TestUtil.scala:93) [238] test.os.watch.WatchTests$.$init$$$anonfun$1$$anonfun$1(WatchTests.scala:60) [238] Tests: 1, Passed: 0, Failed: 1 ``` or randomly crash: ``` [238] -------------------- Running Tests test.os.watch.WatchTests -------------------- [238] X test.os.watch.WatchTests.nonExistentFolder 211ms [238] utest.AssertionError: _root_.os.watch.watch(Seq(wd / "does-not-exist"), onEvent = _ => ()) [238] wd: os.Path = /Users/arturaz/work/os-lib/out/os/watch/jvm/3.3.5/test/test.dest/sandbox/out/scratch/watch/WatchTests/tests/nonExistentFolder [238] utest.asserts.Util$.assertError(Util.scala:20) [238] utest.asserts.Asserts$.interceptImpl(Asserts.scala:49) [238] test.os.watch.WatchTests$.$init$$$anonfun$1$$anonfun$1$$anonfun$1(WatchTests.scala:60) [238] test.os.TestUtil$.prep$$anonfun$1(TestUtil.scala:92) [238] test.os.TestUtil$.mkDir(TestUtil.scala:64) [238] test.os.TestUtil$.prep(TestUtil.scala:93) [238] test.os.watch.WatchTests$.$init$$$anonfun$1$$anonfun$1(WatchTests.scala:60) [238] # [238] # A fatal error has been detected by the Java Runtime Environment: [238] # [238] # SIGSEGV (0xb) at pc=0x000000018409957c, pid=33912, tid=4355 [238] # [238] # JRE version: OpenJDK Runtime Environment Temurin-21.0.1+12 (21.0.1+12) (build 21.0.1+12-LTS) [238] # Java VM: OpenJDK 64-Bit Server VM Temurin-21.0.1+12 (21.0.1+12-LTS, mixed mode, tiered, compressed oops, compressed class ptrs, g1 gc, bsd-aarch64) [238] # Problematic frame: [238] # C [CoreFoundation+0x15057c] __CFCheckCFInfoPACSignature+0x4 [238] # [238] # No core dump will be written. Core dumps have been disabled. To enable core dumping, try "ulimit -c unlimited" before starting Java again [238] # [238] # An error report file with more information is saved as: [238] # /Users/arturaz/work/os-lib/out/os/watch/jvm/3.3.5/test/test.dest/sandbox/hs_err_pid33912.log [238] [0.663s][warning][os] Loading hsdis library failed [238] # [238] # If you would like to submit a bug report, please visit: [238] # https://github.com/adoptium/adoptium-support/issues [238] # The crash happened outside the Java Virtual Machine in native code. [238] # See problematic frame for where to report the bug. [238] # ``` This one randomly fails on `main` as well: ``` [238] X test.os.watch.WatchTests.openClose.manyTimes 4668ms [238] java.util.concurrent.TimeoutException: no file system changes detected within 3 seconds [238] test.os.watch.WatchTests$.testOpenClose$1$$anonfun$1(WatchTests.scala:354) [238] scala.runtime.java8.JFunction1$mcVI$sp.apply(JFunction1$mcVI$sp.scala:18) [238] scala.collection.immutable.Range.foreach(Range.scala:192) [238] test.os.watch.WatchTests$.testOpenClose$1(WatchTests.scala:333) [238] test.os.watch.WatchTests$.$init$$$anonfun$1$$anonfun$5$$anonfun$2$$anonfun$1(WatchTests.scala:367) [238] scala.runtime.function.JProcedure1.apply(JProcedure1.java:15) [238] scala.runtime.function.JProcedure1.apply(JProcedure1.java:10) [238] test.os.TestUtil$.prep$$anonfun$1(TestUtil.scala:92) [238] test.os.TestUtil$.mkDir(TestUtil.scala:64) [238] test.os.TestUtil$.prep(TestUtil.scala:93) [238] test.os.watch.WatchTests$.$init$$$anonfun$1$$anonfun$5$$anonfun$2(WatchTests.scala:60) ``` This one fails because the behaviour has changed: ``` [238] X test.os.watch.WatchTests.closeIsSafeToInvokeMultipleTimes 210ms [238] java.lang.AssertionError: assertion failed [238] scala.runtime.Scala3RunTime$.assertFailed(Scala3RunTime.scala:11) [238] os.watch.FSEventsWatcher.close(FSEventsWatcher.scala:101) [238] test.os.watch.WatchTests$.$init$$$anonfun$1$$anonfun$6$$anonfun$1(WatchTests.scala:379) [238] scala.runtime.function.JProcedure1.apply(JProcedure1.java:15) [238] scala.runtime.function.JProcedure1.apply(JProcedure1.java:10) [238] test.os.TestUtil$.mkDir(TestUtil.scala:64) [238] test.os.watch.WatchTests$.$init$$$anonfun$1$$anonfun$6(WatchTests.scala:60) ``` This should fix the issue witnessed in mill (com-lihaoyi/mill#5225)
1 parent 638ae3b commit 6edd90e

File tree

11 files changed

+934
-343
lines changed

11 files changed

+934
-343
lines changed

.gitignore

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ target/
88
.sbtserver
99
.scala-build/
1010
.bsp/
11+
.bloop
12+
.metals
1113
project/.sbtserver
1214
tags
1315
nohup.out

.vscode/settings.json

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
{
2+
"files.watcherExclude": {
3+
"**/target": true
4+
}
5+
}

os/test/src/TestUtil.scala

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,8 @@ object TestUtil {
3838
str1Normalized == str2Normalized
3939
}
4040

41-
def prep[T](f: os.Path => T)(implicit tp: TestPath, fn: sourcecode.FullName) = {
41+
/** Creates a temporary directory for the current test. */
42+
def mkDir[T](f: os.Path => T)(implicit tp: TestPath, fn: sourcecode.FullName) = {
4243
val segments = Seq("out", "scratch") ++ fn.value.split('.').drop(2) ++ tp.value
4344

4445
val directory = Paths.get(segments.mkString("/"))
@@ -58,6 +59,15 @@ object TestUtil {
5859
}
5960
)
6061

62+
val wd = os.Path(directory.toAbsolutePath)
63+
os.makeDir.all.apply(wd)
64+
f(wd)
65+
}
66+
67+
/** Populates the directory with test resources. */
68+
def populate[T](directory: Path)(implicit tp: TestPath, fn: sourcecode.FullName): Unit = {
69+
if (os.exists(os.Path(directory))) os.remove.all(os.Path(directory))
70+
6171
val original = Paths.get(sys.env("OS_TEST_RESOURCE_FOLDER"), "test")
6272
Files.walkFileTree(
6373
original,
@@ -73,8 +83,14 @@ object TestUtil {
7383
}
7484
}
7585
)
86+
}
7687

77-
f(os.Path(directory.toAbsolutePath))
88+
/** Creates a temporary directory for the current test and populates it with test resources. */
89+
def prep[T](f: os.Path => T)(implicit tp: TestPath, fn: sourcecode.FullName) = {
90+
mkDir { wd =>
91+
populate(wd.toNIO)
92+
f(wd)
93+
}
7894
}
7995

8096
def prepChecker[T](f: os.Path => T)(implicit tp: TestPath, fn: sourcecode.FullName): T =

os/watch/src/CarbonApi.scala

Lines changed: 0 additions & 104 deletions
This file was deleted.

os/watch/src/FSEventsWatcher.scala

Lines changed: 0 additions & 110 deletions
This file was deleted.

0 commit comments

Comments
 (0)