Implement SunOS FileSystemWatcher using portfs event ports#124716
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements FileSystemWatcher support for SunOS (Solaris/illumos) using the portfs (event ports) API. Unlike Linux's inotify which reports specific file changes, portfs only signals that a directory has changed, requiring the implementation to maintain snapshots and perform comparisons to determine what actually changed.
Changes:
- Native interop layer for portfs operations (port_create, port_associate, port_get, port_send, port_dissociate)
- SunOS-specific FileSystemWatcher implementation using snapshot-based change detection
- Hybrid monitoring mode that watches both directories (for name changes) and individual files (for attribute changes)
- Support for recursive subdirectory monitoring with resource limits (50 subdirs, 1000 files per directory)
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| src/native/libs/configure.cmake | CMake configuration for portfs feature detection (has critical bug) |
| src/native/libs/System.Native/pal_io.h | Header declarations for portfs native functions (has spelling errors) |
| src/native/libs/System.Native/pal_io.c | Native implementation of portfs wrapper functions (has critical string lifetime bug) |
| src/native/libs/System.Native/entrypoints.c | Export table entries for new portfs functions |
| src/native/libs/Common/pal_config.h.in | Configuration flag for HAVE_SUNOS_PORTFS |
| src/libraries/System.IO.FileSystem.Watcher/tests/System.IO.FileSystem.Watcher.Tests.csproj | Added illumos and solaris to test target frameworks |
| src/libraries/System.IO.FileSystem.Watcher/src/System/IO/FileSystemWatcher.SunOS.cs | Main SunOS implementation with snapshot comparison and event generation (multiple issues) |
| src/libraries/System.IO.FileSystem.Watcher/src/System.IO.FileSystem.Watcher.csproj | Added SunOS-specific source files and target frameworks |
| src/libraries/Common/src/Interop/SunOS/portfs/Interop.portfs.cs | Managed P/Invoke declarations for portfs (has hardcoded struct size issue) |
cbe385d to
04c54a8
Compare
8b4f6e2 to
3c5ee5f
Compare
|
Added a (platform-specific) workaround to the FileSystemWatcher test: Short delay between create and destroy. With the work-around, all tests pass: |
|
Caught up with Copilot feedback. I'm going to squash and then rebase to pick up recent changes in FileSystemWatcher etc. No code changes other than the squash and rebase. |
This implementation uses SunOS portfs (event ports) to watch for directory changes. Unlike Linux inotify, portfs can only detect WHEN a directory changes, not WHAT changed. Therefore, this implementation: 1. Maintains a snapshot of each watched directory's contents 2. When portfs signals a change, re-reads the directory 3. Compares new vs old snapshots to detect creates/deletes/modifications 4. Generates appropriate FileSystemWatcher events Key implementation details: - Uses a single RunningInstance (event port and thread) per watcher - Uses pinned GC.AllocateArray for file_obj structures required by portfs - When watching attributes or timestamps watches directory contents too - Each watched objct gets a unique cookie for identification - port_get returns the cookie to indicate which object changed - Optimized snapshot comparison with sorted single-pass algorithm - Supports IncludeSubdirectories by recursively watching child directories - Track mtime of the watched directory to avoid missing changes - Implement graceful cancellation using PortSend Native changes (pal_io.c): - SystemNative_PortCreate: Opens event port file descriptor - SystemNative_PortAssociate: Associates directory with port using file_obj - SystemNative_PortGet: Waits for events, returns cookie identifying directory - SystemNative_PortSend: Send an event (used in cancellation) - SystemNative_PortDissociate: Removes directory association Performance notes: - Less efficient than inotify (must re-read directories on each change) - Better than polling (blocks until change occurs) - Memory overhead for directory snapshots --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Rebase and re-test complete. |
…ystemWatcher.SunOS.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
In CompareSnapshotsAndNotify, if the same inode appears in both creates and deletes more than once (e.g. hard links), a second change could pair with an already-processed counterpart and emit a spurious rename instead of an unpaired create/delete. Guard the pairing with !other.Processed.
|
Is there anything more I should be doing to help move this along? Anything? Thanks. |
|
@jkotas, @adamsitnik, could we please get this reviewed/merged? We have OpenBSD next in line #124911 and it would be nice if we could avoid conflicts with this one. 🙂 Without System.Diagnostics.Process, one can't run .NET SDK and some test suites on the target platform. Thanks! cc @sethjackson S.D.P is the most important one of five platform-specific runtime library. |
Gotcha. Hopefully we can do things similar to macOS/FreeBSD. |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "f83c3c1ca67c0ea6f45797439aab667cb8c2ebe4",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "985b17607b1b33d3988af0a4f021455073161ae8",
"last_reviewed_commit": "f83c3c1ca67c0ea6f45797439aab667cb8c2ebe4",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "985b17607b1b33d3988af0a4f021455073161ae8",
"last_recorded_worker_run_id": "29676145327",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "f83c3c1ca67c0ea6f45797439aab667cb8c2ebe4",
"review_id": 4730529856
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: Well justified. FileSystemWatcher currently has no illumos/Solaris implementation, so this fills a real platform gap. Using portfs (event ports) rather than the experimental/non-universal illumos inotify is a sound, well-reasoned choice that the PR documents clearly.
Approach: Reasonable given the constraint that portfs signals that a directory changed but not what changed. The snapshot-and-diff design, single-port/single-thread model, POH-pinned file_obj buffers, cookie-based routing, and the observation-point-mtime re-association trick to avoid missed events are all coherent. The inherent limitations (create+delete within one interval, hard-link rename pairing) are honestly documented.
Summary: PortAssociate time baseline: only fo_mtime is set while fo_atime/fo_ctime are left zeroed, which could cause immediate event re-fire (busy loop) when watching Attributes/Security/CreationTime/LastAccess filters — the default filter set masks this. A reviewer with illumos access should validate that and the ETIME vs ETIMEDOUT semantics. The test change also has a style violation and a timing-based workaround worth scrutiny. None of these are confirmed defects, but they need human/platform verification before merge.
Detailed Findings
⚠️ Correctness — PortAssociate baseline times (atime/ctime zeroed)
See inline comment on pal_io.c. memset zeroes the file_obj and only fo_mtime is populated. For PORT_SOURCE_FILE, the kernel fires immediately when the current atime/mtime/ctime exceeds the supplied baseline. A zero ctime/atime baseline is always exceeded, so associations that include FILE_ATTRIB or FILE_ACCESS may re-fire immediately on every re-association, producing a CPU-spinning loop. The default NotifyFilters (→ FILE_MODIFIED only) hides this, so it would not surface in basic testing. Needs verification on illumos.
⚠️ Correctness — ETIME vs ETIMEDOUT timeout errno
See inline comment on pal_io.c SystemNative_PortGet. port_get(3C) reports timeout via ETIME, but the managed loop treats only ETIMEDOUT as benign. Latent only (all current calls pass tmo == NULL), but worth fixing to avoid a future foot-gun.
⚠️ Test quality — SunOS workaround
See inline comment on FileSystemWatcher.cs. Same-line brace violates .editorconfig C# brace style, and the Thread.Sleep(100) to make a create+delete observable is timing-fragile. The underlying missed-event behavior appears to be an inherent portfs/snapshot limitation (acknowledged in the code's own remarks) rather than a fixable bug, so a targeted delay is defensible, but the magic constant could still flake under load.
⚠️ Testability / CI coverage
There is no illumos/Solaris CI leg in dotnet/runtime that runs these tests, so the ~1900 new lines are effectively unverified by automation. The PR description asserts "Passes all System.IO.FileSystem.Watcher.Tests" — this should be independently confirmed by a maintainer with illumos access, since regressions here cannot be caught by CI.
💡 Minor — FD_CLOEXEC set non-atomically after port_create
See inline comment on pal_io.c. Consistent with existing helpers in the file; minor fd-leak TOCTOU window. Not blocking.
💡 Observation — Debug logging volume
ProcessEvents/HandleEvent and friends contain many Debug.WriteLine calls on the hot event path. These compile out of Release builds, so no shipping impact, but the density is high. Optional cleanup.
✅ Verified — resource ownership and lifecycle
The RunningInstance/WeakReference design, StartRaisingEvents failure cleanup (disposes handle and CTS on any throw), finalizer path calling StopRaisingEvents, and EBADF-quiet shutdown handling are internally consistent and mirror the established Unix inotify pattern in this component. The POH-pinned file_obj buffer usage matches the kernel's requirement that the association object address remain stable.
✅ Verified — no new public API surface
The change adds only internal/private managed members and PALEXPORT native entrypoints wired through entrypoints.c; no ref/ assembly or public members are introduced, so no API-approval gate applies.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 200.5 AIC · ⌖ 12.2 AIC · ⊞ 10K
| // Use the provided mtime from managed code | ||
| // This is the mtime captured before reading the directory | ||
| if (mtime != NULL) { | ||
| fo->fo_mtime.tv_sec = mtime->tv_sec; |
There was a problem hiding this comment.
fo_mtime, leaving fo_atime/fo_ctime zeroed. memset(fo, 0, ...) zeroes the whole file_obj, and only fo_mtime is subsequently populated. For PORT_SOURCE_FILE, the kernel compares the vnode's current atime/mtime/ctime against these baselines and fires the event immediately if the current value is newer. With fo_ctime/fo_atime left at 0, any association that includes FILE_ATTRIB (NotifyFilters.Attributes | Security | CreationTime) or FILE_ACCESS (NotifyFilters.LastAccess) will register with a baseline of 0, so the current ctime/atime (always > 0) exceeds it and the event re-fires immediately on every re-association — a potential 100% CPU busy loop that never blocks in port_get. The default NotifyFilters (LastWrite|FileName|DirectoryName → FILE_MODIFIED only) hides this because only mtime is watched, but attribute/access watching is a supported configuration. Please capture and pass the corresponding ctime/atime baselines (or confirm on illumos that a zero baseline does not trigger immediate re-fire). I could not validate this on an illumos host, so treat it as a concern to verify rather than a confirmed defect.
| (void)port, (void)pFileObj; | ||
| errno = ENOTSUP; | ||
| return -1; | ||
| #endif |
There was a problem hiding this comment.
SystemNative_PortGet returns -1 on any port_get failure, but the managed ProcessEvents loop specifically treats ETIMEDOUT as a benign continue. On illumos port_get(3C) sets errno to ETIME (not ETIMEDOUT) when the timeout expires. In this PR PortGet is always called with tmo == NULL (indefinite wait), so the timeout path is currently unreachable and this is latent rather than an active bug — but if a timeout is ever introduced, the ETIMEDOUT check in ProcessEvents (FileSystemWatcher.SunOS.cs) would not match and the timeout would be surfaced as an error. Consider normalizing ETIME→ETIMEDOUT here, or checking for Interop.Error.ETIME in the managed loop.
| #if HAVE_SUNOS_PORTFS | ||
| int32_t fd = port_create(); | ||
| if (fd != -1) { | ||
| fcntl(fd, F_SETFD, FD_CLOEXEC); |
There was a problem hiding this comment.
💡 The fcntl(fd, F_SETFD, FD_CLOEXEC) return value is ignored, and there is a TOCTOU window between port_create() and setting FD_CLOEXEC during which a concurrent fork/exec could leak the fd. Other fd-creating helpers in this file (e.g. the pipe/Open paths) also set FD_CLOEXEC non-atomically, so this is consistent with existing code and only a minor concern — but if illumos exposes a port_create flag or an atomic alternative, prefer it. Not blocking.
| { | ||
| var tempFile = new TempFile(Path.Combine(TestDirectory, GetTestFileName())); | ||
| // SunOS needs a short delay or the create/remove will not be seen. | ||
| if (PlatformDetection.IsSunOS) { |
There was a problem hiding this comment.
-
Brace style:
if (PlatformDetection.IsSunOS) {uses same-line braces, which violates the repo's C# convention (Allman/next-line braces enforced by.editorconfig). Please move the{to its own line. -
Masking a real product limitation in the test: inserting
Thread.Sleep(100)so a create-then-delete becomes observable adjusts the test to fit the implementation. Because portfs only reports that a directory changed and the code re-reads/diffs snapshots, a create+delete within one snapshot interval is legitimately invisible (the PR's ownCompareSnapshotsAndNotifyremarks acknowledgecreate A; rm A"may not be visible at all"). That is an inherent, documented limitation, so a targeted delay is defensible — but a magic100ms sleep is timing-fragile and could still flake under load. Consider a comment linking to the limitation and/or a more robust synchronization approach. At minimum, confirm this doesn't just paper over a fixable missed-event bug.
This implementation uses SunOS portfs (event ports) to watch for directory changes. Unlike Linux inotify, portfs can only detect WHEN a directory changes, not WHAT changed. Therefore, this implementation:
Key implementation details:
Native changes (pal_io.c):
Performance notes:
Passes all System.IO.FileSystem.Watcher.Tests