-
Notifications
You must be signed in to change notification settings - Fork 246
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
Issue #481 fix hanging race condition #482
Issue #481 fix hanging race condition #482
Conversation
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.
Thanks for your effort. I have asked for some changes.
In the future, please make smaller PRs focusing on one particular issue.
Pay attention to changes to public interfaces since this can cause a lot of pain downstream.
I look forward to reviewing and merging this PR and others from you.
lib/listen/event/loop.rb
Outdated
class Error < RuntimeError | ||
class NotStarted < Error | ||
end | ||
class ThreadFailedToStart < Error; end |
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.
Since NotStarted
is a public interface, please reuse it instead of introducing ThreadFailedToStart
.
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.
Good point--I hadn't thought about it being in the public interface. I will put that back to its old name.
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 restored it to its old name, NotStarted
. And if start
is called when we're already started, I put the code back as it was to simply return as a no-op, since there could be callers who were counting on that public behavior.
0b236c1
lib/listen/event/loop.rb
Outdated
|
||
def start | ||
transition! :starting do | ||
state == :pre_start or raise Error::AlreadyStarted |
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 would say it's better to use explicit if statement here
unless state == :pre_start
raise Error::AlreadyStarted...
end
lib/listen/event/loop.rb
Outdated
class NotStarted < Error | ||
end | ||
class ThreadFailedToStart < Error; end | ||
class AlreadyStarted < Error; end |
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 wonder if this should inherit from NotStarted
to preserve existing semantics where possible.
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.
That might be a bit confusing though?
AlreadyStarted.is_a?(NotStarted)
I will look into what was raised before if start
was called a second time.
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.
Yes, understood. In this context... if someone calls start, and it's already started, that's a kind of NotStarted
because it was already started. If you can think of something better, it's worth considering.
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.
Restored to its old behavior: 0b236c1
# Passing a state name sets the start state | ||
def start_state(new_start_state = nil) | ||
if new_start_state | ||
new_start_state.is_a?(Symbol) or raise ArgumentError, "state name must be a Symbol (got #{new_start_state.inspect})" |
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.
Instead of explicitly detecting it's a symbol, why not use to_sym
?
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 is a style we like for enforcing contracts, especially around symbol-vs-string! There aren't any callers of this method passing strings, so .to_sym
would be a no-op now. Adding .to_sym
now might serve to enable more confusing usage later...but there's no need for that, is there?
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 kind of pseudo type checking is uncommon in Ruby, even thought I can see how it makes sense.
The reality is, if you add this, there are many other places where you might start trying to enforce type contracts.
Agreed, we don't really want users passing in strings.
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.
True that it's not common in Ruby (yet). But we've found it to be invaluable for containing String vs. Symbol insanity! Otherwise, you wind up with defensive code sprinkled everywhere that tries to handle String and Symbol as equivalent...and you never know if that clutter is complete.
Speaking of which, I added the missing spec for the FSM module and made sure to test this case: bbd10ee
lib/listen/fsm.rb
Outdated
# Low-level, immediate state transition with no checks or callbacks. | ||
def transition!(new_state_name) | ||
new_state_name.is_a?(Symbol) or raise ArgumentError, "state name must be a Symbol (got #{new_state_name.inspect})" | ||
@mutex or raise ArgumentError, "FSM not initialized. You must call super() from initialize!" |
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 seems like an overly specific issue to check for - I can imagine some code was not working correctly, but in that case @mutex.synchronize
will blow up with nil error... do we need to manually check 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.
I added this because of the contract that the including code must call super()
first. That's pretty unusual for mixins, but it's how the code was already structured, and it work well in this case. It's critical to get the mutex initialized up front because the lazy style that's often used in mixins (@mutex ||= Mutex.new
) wouldn't work; ironically, it would need a synchronizing mutex around it!
Ah, this might make it more clear: how about an overall flag like @_fsm_initialized
that's set = true
when super()
is called. This could code check that for the ArgumentError
test. If that test passes, we can be confident that @mutex
has been initialized.
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.
Understood.
Well, I don't really want to make the code any more complicated than it already is.
If the code is not implemented correctly (missing call to super
) it's a mechanical problem - i.e. the code needs to be fixed. This isn't a transient run-time issue. The current implementation is okay, but it's also very explicit in a non-canonical way.
I don't think we should be adding extra instance variables to enforce initialisation.
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 realized we had to move away from super()
. There's a reason I've never seen mixins initialized that way before: it precludes any base class initialization, since the mixin would have to do that for you (by calling super
itself), but would have no way to know the base class's initialize
signature!
Instead I went with an explicitly-named method initialize_fsm
. And stored a variable to remember if that didn't happen. And I addressed the longstanding comment in there to make the FSM methods private. And I added a spec for the FSM module and tested all of this. I think it's all much cleaner now! LMK what you think.
@name = name | ||
@block = block | ||
@transitions = nil | ||
@transitions = Array(transitions).map(&:to_sym) if transitions | ||
@transitions = if transitions |
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 don't need if transitions
.
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 kept it from the old code. You can see it hiding up there one line up... in "trailing if" form. There is code below that counts on it. It's just after this comment:
All transitions are allowed if none are expressly declared
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.
It's simply not needed. Array(nil) => []
.
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 don't follow. That if
is there so that when transitions
is nil
, @transitions
will be as well. The code below counts on that, because it uses nil
in @transitions
as an indicator that all transitions are allowed.
You may need to rebase on master. |
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.
Thanks for the review, @ioquatix.
I've responded to all the comments you left. 3 of them require changes. I'll try to push those up in the next day or two.
lib/listen/event/loop.rb
Outdated
class Error < RuntimeError | ||
class NotStarted < Error | ||
end | ||
class ThreadFailedToStart < Error; end |
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.
Good point--I hadn't thought about it being in the public interface. I will put that back to its old name.
lib/listen/event/loop.rb
Outdated
class NotStarted < Error | ||
end | ||
class ThreadFailedToStart < Error; end | ||
class AlreadyStarted < Error; end |
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.
That might be a bit confusing though?
AlreadyStarted.is_a?(NotStarted)
I will look into what was raised before if start
was called a second time.
lib/listen/event/loop.rb
Outdated
|
||
def start | ||
transition! :starting do | ||
state == :pre_start or raise Error::AlreadyStarted |
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.
We're fans for the one-line and
/or
form for concise preconditions [this use case is exactly why and
and or
are in Ruby, from Perl], but I can certainly change it to the 3-line form.
# Passing a state name sets the start state | ||
def start_state(new_start_state = nil) | ||
if new_start_state | ||
new_start_state.is_a?(Symbol) or raise ArgumentError, "state name must be a Symbol (got #{new_start_state.inspect})" |
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 is a style we like for enforcing contracts, especially around symbol-vs-string! There aren't any callers of this method passing strings, so .to_sym
would be a no-op now. Adding .to_sym
now might serve to enable more confusing usage later...but there's no need for that, is there?
lib/listen/fsm.rb
Outdated
# Low-level, immediate state transition with no checks or callbacks. | ||
def transition!(new_state_name) | ||
new_state_name.is_a?(Symbol) or raise ArgumentError, "state name must be a Symbol (got #{new_state_name.inspect})" | ||
@mutex or raise ArgumentError, "FSM not initialized. You must call super() from initialize!" |
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 added this because of the contract that the including code must call super()
first. That's pretty unusual for mixins, but it's how the code was already structured, and it work well in this case. It's critical to get the mutex initialized up front because the lazy style that's often used in mixins (@mutex ||= Mutex.new
) wouldn't work; ironically, it would need a synchronizing mutex around it!
Ah, this might make it more clear: how about an overall flag like @_fsm_initialized
that's set = true
when super()
is called. This could code check that for the ArgumentError
test. If that test passes, we can be confident that @mutex
has been initialized.
@name = name | ||
@block = block | ||
@transitions = nil | ||
@transitions = Array(transitions).map(&:to_sym) if transitions | ||
@transitions = if transitions |
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 kept it from the old code. You can see it hiding up there one line up... in "trailing if" form. There is code below that counts on it. It's just after this comment:
All transitions are allowed if none are expressly declared
@@ -0,0 +1,103 @@ | |||
RSpec.describe Listen::FSM do |
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.
Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.
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 will make a separate PR for this later.
4a0458a
to
d7efb56
Compare
d7efb56
to
17c612d
Compare
Hi @ioquatix |
My apologies, I was busy for Ruby Kaigi conference. I'm reviewing it now. |
I'll fix the Ruby 3 issues. However can you check why:
|
Okay, I see a bunch of specs are failing, I'll try to bring master back to green. |
Okay, master is green again, can you rebase? |
92a532c
to
b4c5afe
Compare
@ioquatix Thanks for getting |
… :started, :stopped
…tops the suite)
… sleep()/wakeup
…fault_state to start_state
…there are multiple state machines)
…) instead of _sleep
… instance method that just delegate to class, require symbols
…ure multiple threads are waiting
… called in @fsm_initialized
…tStarted), and silently no-op on start if already started
b4c5afe
to
24dec52
Compare
@ioquatix : Rebased and pushed. |
Great work, thanks for your effort. Do you have time to jump on call some time this week to discuss a release plan/schedule? |
Hi @ioquatix Thank you! Yes, this week is good for a call. I'm on |
Fixes issue #481 by removing code that attempted to synchronize threads with
sleep()
andThread#wakeup
. That was a race condition bug since ifwakeup
ran beforesleep()
, it would be lost and the sleeping thread would simply hang.Detailed changes:
:Mutex
/::ConditionVariable
to wait/wake up on state transitions. I factored this down into FSM since it's used in two state machine classes now.FSM
. Enforce symbol contracts, remove unnecessary methods, renamedefault_state
tostart_state
since the latter is standard FSM terminology. Also initialize withinitialize_fsm
vssuper()
.Queue#pop
to block front-end when waiting for events.Loop#setup
andLoop#resume
since they were always called consecutively. This allowed theListener
:front_end_ready
state to disappear.Loop#resume
andLoop#pause
since they were no-ops.teardown
tostop
to be consistent. (At some point I'd like to do a similar thing with:processing_events
. This could simply be called:started
.)