feat: Mac port #183 #300
Open
feat: Mac port #183 #300
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
possibly the same in BSD
basically the same according to manual
File system watchers (inotify, FSEvents) report canonical paths, but ConfigWatcher was storing paths as provided by the caller. When a path contained symlinks (e.g., /var -> /private/var on macOS), event paths wouldn't match stored paths, causing config changes to be missed. Add canonicalize_path() to resolve symlinks in the existing portion of a path (handling paths where the final components don't exist yet), and apply it when storing paths in the watcher. Include a regression test that creates a symlink and verifies events are received when watching through the symlink.
The worker_ready() test helper signals when the worker is idle, but it was signaling even when there was a pending reload_deadline. This could cause tests to proceed before the debounce timeout fired, leading to flaky test results. Only signal idle when there's no pending work (reload_deadline is None). Update the debounce test to not call worker_ready() between writes, as that now correctly waits for the reload to complete.
This version includes macOS support via ptsname (vs ptsname_r). See: shell-pool/shpool_pty#4
- Forward test_hooks feature from shpool to libshpool crate - Shorten socket paths to avoid macOS 104-byte sun_path limit - Forward HOME and PATH env vars to attach subprocess (macOS typically lacks XDG_RUNTIME_DIR, needs HOME instead) - Use std::env::var directly to avoid unused import warning - Document that tests should run with --test-threads=1 on macOS due to FSEvents timing behavior under concurrent load
macOS returns EINVAL when setting socket timeout on a connection where the peer has already closed (documented in setsockopt(2)). This typically happens with daemon presence probe connections.
Open
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This work is based on the previous work by @seruman in their fork https://github.com/seruman/shpool/tree/macos
I tried to retain as much of the existing work done and base my work on top of that.
seruman's fork had socket timeouts disabled on macOS. I wasn't able to reproduce socket timeouts not working on macOS but I was seeing unhandled EINVAL errors on daemon presence probe connections (where the peer closes immediately). These are now handled gracefully, so I have dropped the changes that disabled socket timeouts.
I also ran into a bug where ConfigWatcher was not using canonicalized paths, but inotify et. al. use canonicalized paths. This likely wasn't being ran into much on Linux environments but becomes very obvious on macOS because /tmp is a symlink.
Timing on macOS is indeed tricky, the fix would involve probably significantly re-working the way test are being done, but the workaround I found was to run them in serial.
I think the rest of the changes were relatively minor.
Here's a summary of the changes:
and the cherry-picked commits from https://github.com/seruman/shpool/tree/macos