-
Notifications
You must be signed in to change notification settings - Fork 21.3k
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.
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
Make EventedFileUpdateChecker garbage collectable #40133
Make EventedFileUpdateChecker garbage collectable #40133
Conversation
647cc12
to
adf5102
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've only skimmed this and I trust you 🙌💪
adf5102
to
7dbfc43
Compare
When an `EventedFileUpdateChecker` instance detects a watched directory that previously did not exist, it calls `shutdown!` and then `boot!` to reinitialize its underlying listener. Prior to this commit, `shutdown!` invoked `Listen.stop` which stops **all** listeners, globally. This commit changes `shutdown!` to stop the checker's listener only. Fixes rails#38174.
7dbfc43
to
fc2a6c8
Compare
The Listen gem dispatches "file changed" events from a separate thread. This thread holds a reference to the `changed` callback, and runs until the listener is stopped. Thus, objects that are part of the `changed` callback's scope cannot be garbage collected until the listener is stopped. This commit isolates the `changed` callback and associated scope in an `EventedFileUpdateChecker::Core` instance. This ensures that the `EventedFileUpdateChecker` instance can be garbage collected. Additionally, this commit adds a finalizer to the `EventedFileUpdateChecker` instance, which stops the listener and ensures that the `EventedFileUpdateChecker::Core` instance can be garbage collected.
fc2a6c8
to
f8b9150
Compare
wait # Wait for listener thread to start processing events. | ||
GC.start |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test uncovered a race condition in Listen / rb-inotify (see guard/rb-inotify#100) that manifests here as a hang when the finalizer stops the listener. However, I don't think it's likely that a normal application would trigger the race condition, so I think this PR is safe to merge. (I've added a wait
here to prevent the test itself from triggering the race condition.)
end | ||
end | ||
def common_path(paths) | ||
paths.map { |path| path.ascend.to_a }.reduce(&:&)&.first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Behold the three meditating munks! Bringing offerings of :, ) and . &:&)&.
😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little bit of Ruby poetry! 😆
The Listen gem dispatches "file changed" events from a separate thread. This thread holds a reference to the
changed
callback, and runs until the listener is stopped. Thus, objects that are part of thechanged
callback's scope cannot be garbage collected until the listener is stopped.This commit isolates the
changed
callback and associated scope in anEventedFileUpdateChecker::Core
instance. This ensures that theEventedFileUpdateChecker
instance can be garbage collected. Additionally, this commit adds a finalizer to theEventedFileUpdateChecker
instance, which stops the listener and ensures that theEventedFileUpdateChecker::Core
instance can be garbage collected.This is based on top of #40100.
I split this PR into two commits to make it easier to review. The first commit is a minimal diff to make
EventedFileUpdateChecker
garbage collectable. The second commit is cleanup that eliminatesEventedFileUpdateChecker::PathHelper
, instead of fixing its indentation.