Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Always use fs.watch to determine when logfiles have changed #83

Open
wearhere opened this issue Sep 19, 2019 · 1 comment
Open

Always use fs.watch to determine when logfiles have changed #83

wearhere opened this issue Sep 19, 2019 · 1 comment
Labels
enhancement New feature or request

Comments

@wearhere
Copy link
Contributor

This tracks reverting #82 (comment), once the underlying Node issue (probably nodejs/node#29460) has been fixed.

(To be precise, we won't be able to do a straight revert, rather will need to check the Node version and re-enable fs.watch in >= whatever version Node lands the fix.)
@wearhere wearhere added the enhancement New feature or request label Sep 19, 2019
wearhere added a commit that referenced this issue Sep 19, 2019
@wearhere
Fix logfile tailing under Node 10.16.0
5caea0c 
`fs.watch` does not consistently emit `'change'` events on Node 10.16.0,
possibly due to nodejs/node#29460. `fs.watchFile`
still works, but is less efficient, per the docs, so fall back to that only if necessary and also suggest that users run custody under an older version of Node.

I explored several Node file-watching libraries (e.g. chokidar, sane) to try to
find a more performant solution for Node 10.16.0 and it seems that they pretty
much all fall back to `fs.watch` / `fs.watchFile`, unfortunately. sane offers
the option to use watchman, but that requires installing a daemon so is a
non-starter.

Fixes #82.

#83 tracks reverting this when
the underlying Node issue has been fixed.

Tested that log files update both when running custody under v8.9.3 and
v10.16.0, and that `node-tail` only uses `fs.watchFile` in the latter.
@wearhere wearhere mentioned this issue Sep 19, 2019
Merged
@wearhere
Copy link
Contributor Author

wearhere commented Dec 6, 2019

Woo a fix has been released as part of Node >= v12.13.1 so at this point we could re-enable fs.watch in those versions. Not super keen on doing that myself until Mixmax upgrades to such a version but would welcome PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@wearhere