Issue #481 fix hanging race condition - Githubissues

guard / listen

The Listen gem listens to file modifications and notifies you about the changes.

https://rubygems.org/gems/listen

MIT License

1.92k stars 246 forks source link

Issue #481 fix hanging race condition #482

Closed ColinDKelley closed 3 years ago

ColinDKelley commented 4 years ago

Fixes issue #481 by removing code that attempted to synchronize threads with sleep() and Thread#wakeup. That was a race condition bug since if wakeup ran before sleep(), it would be lost and the sleeping thread would simply hang.

Detailed changes:

Use ::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.
Clean up and simplify FSM. Enforce symbol contracts, remove unnecessary methods, rename default_state to start_state since the latter is standard FSM terminology. Also initialize with initialize_fsm vs super().
Use Queue#pop to block front-end when waiting for events.
Combine Loop#setup and Loop#resume since they were always called consecutively. This allowed the Listener :front_end_ready state to disappear.
Remove Loop#resume and Loop#pause since they were no-ops.
Rename teardown to stop to be consistent. (At some point I'd like to do a similar thing with :processing_events. This could simply be called :started.)

coveralls commented 4 years ago

Coverage decreased (-0.7%) to 97.026% when pulling aabfbbfda5e0b040eb05dad429a2e9c097919034 on Invoca:issue_481-fix-hanging-race-condition into ba5059c9f3a8594eae7e0050687c1a6735046d85 on guard:master.

ioquatix commented 4 years ago

You may need to rebase on master.

ColinDKelley commented 4 years ago

@ioquatix I believe all comments are addressed here. I merged up latest master as well. There were a couple build failures that came across with master. I sent a separate PR https://github.com/guard/listen/pull/488 for those and merged it in here.

ColinDKelley commented 4 years ago

Hi @ioquatix Anything else to do on this PR?

ioquatix commented 4 years ago

My apologies, I was busy for Ruby Kaigi conference. I'm reviewing it now.

ioquatix commented 4 years ago

I'll fix the Ruby 3 issues. However can you check why:

Failures:

  1) Listen with one listen dir force_polling option to false with default ignore options two dirs with files in listen dir listens to dir move
     Failure/Error:
       expect(wrapper.listen do
         mv 'dir1', 'dir2/'
       end).to eq expected

       expected: {:added=>["dir2/dir1/file1.rb"], :modified=>[], :removed=>["dir1/file1.rb"]}
            got: {:added=>["dir2/dir1/file1.rb"], :modified=>["dir2/file2.rb"], :removed=>["dir1/file1.rb"]}

       (compared using ==)

ioquatix commented 4 years ago

Okay, I see a bunch of specs are failing, I'll try to bring master back to green.

ioquatix commented 4 years ago

Okay, master is green again, can you rebase?

ColinDKelley commented 3 years ago

@ioquatix Thanks for getting master green again. I've merged that in and now this branch is green too!

ioquatix commented 3 years ago

Can you please rebase on master so I can merge - please don't merge master into your branch because it creates a huge mess.

ColinDKelley commented 3 years ago

@ioquatix : Rebased and pushed.

ioquatix commented 3 years ago

Great work, thanks for your effort. Do you have time to jump on call some time this week to discuss a release plan/schedule?

ColinDKelley commented 3 years ago

Hi @ioquatix Thank you! Yes, this week is good for a call. I'm on US/Pacific time. How about if we pick an exact time and medium by email?