-
Notifications
You must be signed in to change notification settings - Fork 58
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 watch_file into state machine and leverage it throughout #74
Conversation
…uit thread was done
… instances add instance setting for max_open_files
end | ||
|
||
MAX_OPEN_FILES = ENV.fetch("MAX_OPEN_FILES", 4095).to_i |
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.
why is this an environment variable vs a config?
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.
Its both. ENV for when the config is not available to the user.
There are precedents in using both: ENV["SINCEDB_PATH"]
In watch.rb:
...
@max_active = MAX_OPEN_FILES
...
def max_open_files=(value)
return if value.nil?
val = value.to_i
@max_active = val <= 0 ? MAX_OPEN_FILES : value
end
and in tail_base.rb:59
@watch.max_open_files = @opts[:max_open_files]
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.
But it needs a test and a better name because its in the global namespace and it may be overwritten by something else loading later.
listener.accept(line) | ||
@sincedb[@statcache[path]] += (line.bytesize + delimiter_byte_size) |
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.
is there a reason to move this? this line is sort of the "ack" after processing. I believe it was purposefully put after the accept to prevent updating the @sincedb
then suffer some nasty behaviour in accept and essentially skip that line.
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.
You are correct. `listener.accept(line) may raise an error. I will revert this.
running the specs failed for me (ruby mri 2.2.1):
NOTE: it is intermittent for me, can't always replicate |
Thread.new do | ||
sleep quit_sleep | ||
subject.quit | ||
formatted_puts("subscribing") |
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 line is somewhat annoying when running the specs:
~/projects/ruby-filewatch (git)-[pr/74] % bundle exec rspec
....................W, [2016-01-21T10:42:09.185316 #96086] WARN -- : failed to open /var/folders/4g/v6scxgk106dcnlhspm54rl440000gn/T/studtmp-ec52acd1e1143e9af5432021bf1b107bc53c8077cf0a6b11c67cfe9ec5d9: No such file or directory @ rb_sysopen - /var/folders/4g/v6scxgk106dcnlhspm54rl440000gn/T/studtmp-ec52acd1e1143e9af5432021bf1b107bc53c8077cf0a6b11c67cfe9ec5d9
............ subscribing
. subscribing
. subscribing
. subscribing
. subscribing
. subscribing
. subscribing
. subscribing
. subscribing
. subscribing
. subscribing
. subscribing
. subscribing
. subscribing
F subscribing
.
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 can fix this.
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.
should be fixed now.
I can't get it to fail on MRI 2.2.2. |
file_deleteable = [] | ||
# look at the closed to see if its changed | ||
|
||
@files.values.select {|wf| wf.closed? }.each do |watched_file| |
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.
If you're worried about object churn here, you can use @files.select {|_, wf| wf.closed? }.each do |_, wf| ... end
since Hash#select returns an enumerable.
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.
Hash#select returns an enumerable
It does and over a new smaller Hash that the each
iterates over. What I really want is each_selected_value(proc, block)
😄
end | ||
end | ||
|
||
@files.each do |path, watched_file| | ||
# warning on open file limit |
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.
Let's log the number of files that we can't watch becuse it would exceed @max_active, just to let users know that this is an intentional decision by the library and not a bug.
@jsvd I ran the specs a few times and wasn't able to reproduce your failure. Bonus points? :P |
Tested the logstash file input just in case, tests pass ✅
|
# can be called from different threads. | ||
@lock = Mutex.new | ||
# we need to be threadsafe about the quit mutation | ||
@quit = false | ||
@quit_lock = Mutex.new | ||
@max_active = ENV.fetch("FILEWATCH_MAX_OPEN_FILES", 4095).to_i |
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.
My feeling is that this shouldn't be configurable from the environment. However, we can leave it as-is as long as we don't document it.
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.
Maybe mild refactor this to use self.max_active = ...
LGTM |
Make watch_file into state machine and leverage it throughout
NOTES:
@sincedb[@statcache[path]]
returned nil.This is a big refactor.
@statcache
and separate@files
storage hashes are gone.File::Stat
calls are reduced, one during discovery when the WatchFile instance is being constructed and one during the each iteration.closed
,ignored
,watched
,active
. A WatchedFile in anunwatched
state is never looked at again.watched
iteration takes a limited number of watched_files (4096), makes them active and yields:create or :create_initial
but only if the watched_file does not have a history of being closed or ignored before. The implications of this is that if the close_older value is not set to close files then only the first MAX_OPEN_FILES can be tailed. There is an ENV setting for this value as it is set system wide for all instances of filewatch (e.g. multiple file inputs) and an options setting that gives each filewatch instance its own setting.