Permalink
Browse files

make the @Updated flag atomic in the evented monitor

listen is calling us from its own thread, we need to synchronize reads and writes to this flag.
  • Loading branch information...
fxn committed Nov 21, 2015
1 parent 96cc2e8 commit 49a5b408c9e23b937e93f6355b7b0a49a4a23184
Showing with 11 additions and 5 deletions.
  1. +11 −5 activesupport/lib/active_support/file_evented_update_checker.rb
@@ -1,6 +1,8 @@
require 'listen'
require 'set'
require 'pathname'
require 'thread'
require 'concurrent/atomic/atomic_boolean'
module ActiveSupport
class FileEventedUpdateChecker #:nodoc: all
@@ -14,22 +16,24 @@ def initialize(files, dirs = {}, &block)
end
@block = block
@updated = false
@updated = Concurrent::AtomicBoolean.new(false)
@lcsp = @ph.longest_common_subpath(@dirs.keys)
if (dtw = directories_to_watch).any?
Listen.to(*dtw, &method(:changed)).start
end
@mutex = Mutex.new
end
def updated?
@updated
@updated.true?
end
def execute
@block.call
ensure
@updated = false
@updated.make_false

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Nov 21, 2015

Member

@fxn shouldn't this happen before we execute the block? If something changes while we're executing, we need to do another reload next time.

@matthewd

matthewd Nov 21, 2015

Member

@fxn shouldn't this happen before we execute the block? If something changes while we're executing, we need to do another reload next time.

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Nov 21, 2015

Member

The order was copied from

def execute
@last_watched = watched
@last_update_at = updated_at(@last_watched)
@block.call
ensure
@watched = nil
@updated_at = nil
end
.

@josevalim do you remember if there was a reason to do it that way?

@fxn

fxn Nov 21, 2015

Member

The order was copied from

def execute
@last_watched = watched
@last_update_at = updated_at(@last_watched)
@block.call
ensure
@watched = nil
@updated_at = nil
end
.

@josevalim do you remember if there was a reason to do it that way?

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Nov 21, 2015

Member

After thinking about it, I'd say in the original monitor the order wouldn't matter, since the flag won't be checked until the next run. I wonder about the ensure block though, it is not something you normally write without a justification, but cannot see a reason not to reset the ivars before the callback invocation.

In our case things are async. Yes, unless I am missing something, I think the flag should be better updated before the callback invocation.

@fxn

fxn Nov 21, 2015

Member

After thinking about it, I'd say in the original monitor the order wouldn't matter, since the flag won't be checked until the next run. I wonder about the ensure block though, it is not something you normally write without a justification, but cannot see a reason not to reset the ivars before the callback invocation.

In our case things are async. Yes, unless I am missing something, I think the flag should be better updated before the callback invocation.

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Nov 21, 2015

Member

Revised in 4596c1a. If there's anything overlooked let's revert, but looks right to me AFAICT.

@fxn

fxn Nov 21, 2015

Member

Revised in 4596c1a. If there's anything overlooked let's revert, but looks right to me AFAICT.

end
def execute_if_updated
@@ -42,8 +46,10 @@ def execute_if_updated
private
def changed(modified, added, removed)
unless updated?
@updated = (modified + added + removed).any? { |f| watching?(f) }
@mutex.synchronize do
unless updated?
@updated.value = (modified + added + removed).any? { |f| watching?(f) }
end
end

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Nov 21, 2015

Member

AFAICS, this should be fine as unless updated?; @updated.make_true if (modified + added + removed).any? { |f| watching?(f) }; end, without the mutex.

@matthewd

matthewd Nov 21, 2015

Member

AFAICS, this should be fine as unless updated?; @updated.make_true if (modified + added + removed).any? { |f| watching?(f) }; end, without the mutex.

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Nov 21, 2015

Member

I think there would be a race condition without the mutex.

Let's suppose @updated is false. Two parallel threads call the method and pass the predicate check, so both enter the body of unless. The thread that gets to run first there sets the flag to true, but the second one sets it to false. In that scenario the object misses there was an update.

@fxn

fxn Nov 21, 2015

Member

I think there would be a race condition without the mutex.

Let's suppose @updated is false. Two parallel threads call the method and pass the predicate check, so both enter the body of unless. The thread that gets to run first there sets the flag to true, but the second one sets it to false. In that scenario the object misses there was an update.

This comment has been minimized.

Show comment
Hide comment
@fxposter

fxposter Nov 22, 2015

Contributor

@fxn, I agree with @matthewd, actually. Your example is incorrect in this part: "but the second one sets it to false". @matthewd proposes not toset it to false in changed method ever. IE: if something is changed - set to true, if not - do nothing.

There is a race condition, but if we end up with setting the @updated variable only to true here - it doesn't matter. IE: at least one thread will set it to true, and if all other methods will set it to true again - we don't care.

The method can be simplified to:

@updated.make_true if (modified + added + removed).any? { |f| watching?(f) }

And the unless updated? can be added only to skip all those watching? checks (ie: it is actually not essential for the method to work). I tried to think about the implications of having not synchronized read and write of the variable and in this case I couldn't find any problems with it.

@fxposter

fxposter Nov 22, 2015

Contributor

@fxn, I agree with @matthewd, actually. Your example is incorrect in this part: "but the second one sets it to false". @matthewd proposes not toset it to false in changed method ever. IE: if something is changed - set to true, if not - do nothing.

There is a race condition, but if we end up with setting the @updated variable only to true here - it doesn't matter. IE: at least one thread will set it to true, and if all other methods will set it to true again - we don't care.

The method can be simplified to:

@updated.make_true if (modified + added + removed).any? { |f| watching?(f) }

And the unless updated? can be added only to skip all those watching? checks (ie: it is actually not essential for the method to work). I tried to think about the implications of having not synchronized read and write of the variable and in this case I couldn't find any problems with it.

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Nov 22, 2015

Member

Oh yes, I misread @matthewd comment yesterday.

@fxn

fxn Nov 22, 2015

Member

Oh yes, I misread @matthewd comment yesterday.

This comment has been minimized.

Show comment
Hide comment
@fxn

fxn Nov 22, 2015

Member

Revised in 0b9812b.

@fxn

fxn Nov 22, 2015

Member

Revised in 0b9812b.

This comment has been minimized.

Show comment
Hide comment
@fxposter

fxposter Nov 22, 2015

Contributor

Great, thanks!

@fxposter

fxposter Nov 22, 2015

Contributor

Great, thanks!

end

0 comments on commit 49a5b40

Please sign in to comment.