Skip to content
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

listen uses Time.now for time intervals; should use Monotonic tick count #510

Closed
ColinDKelley opened this issue Nov 7, 2020 · 8 comments · Fixed by #512
Closed

listen uses Time.now for time intervals; should use Monotonic tick count #510

ColinDKelley opened this issue Nov 7, 2020 · 8 comments · Fixed by #512
Labels
Milestone

Comments

@ColinDKelley
Copy link
Collaborator

ColinDKelley commented Nov 7, 2020

Current State

The 'listen gem uses the Time.now clock for measuring time intervals. For example, from base.rb:

      def _wait_until_events_calm_down
        loop do
          now = _timestamp

          # Assure there's at least latency between callbacks to allow
          # for accumulating changes
          diff = _deadline - now
          break if diff <= 0

          # give events a bit of time to accumulate so they can be
          # compressed/optimized
          _sleep(diff)
        end
      end

...
      def _timestamp
        config.timestamp
      end
...
      def timestamp
        Time.now.to_f
      end

This is a bug since the Time.now timer may be updated at any time, forward or backward. Most commonly by ntp.

Desired State

The 'listen gem should use the Monotonic tick count instead for time intervals. There are several gems that encapsulate this, including https://github.com/Freaky/monotime, https://github.com/invoca/monotonic_tick_count. Perhaps best for portability and minimal coupling, concurrent-ruby has a simple encapsulation as monotonic_time.

Steps to Reproduce

I didn't try, but you could reproduce the bug by adjusting the system clock while listen is running. If you adjusted the clock forward by, say, 1 second, you could trick the _wait_until_events_calm_down method into thinking it didn't need to wait at all, thus leading to duplicate events.

@ColinDKelley ColinDKelley assigned ioquatix and unassigned ioquatix Nov 7, 2020
@ColinDKelley
Copy link
Collaborator Author

@ioquatix Curious to get your opinion here.

@ioquatix
Copy link
Member

ioquatix commented Nov 7, 2020

Just use the Ruby interface don't pull in a dependency for it.

@ColinDKelley
Copy link
Collaborator Author

Sounds good to avoid dependencies. I wonder if we'll have trouble with some Ruby versions not supporting Process::CLOCK_MONOTONIC? concurrent-ruby has code that adapts for that not being defined in MRI, and a special case that talks to the JVM in JRuby. I suppose that could just be outdated? If we do hit it, we could copy that if/elsif/else logic in here, I suppose.

@ioquatix
Copy link
Member

ioquatix commented Nov 8, 2020

You can ignore any version that doesn't support it, they should all be EOL.

@ColinDKelley
Copy link
Collaborator Author

Sounds good.
BTW this is a minor bug in the scheme of things, so I'd like to release v3.3.0 before fixing this.

@ioquatix
Copy link
Member

ioquatix commented Nov 8, 2020

Sure, if you are happy with that, I am happy with it. It's no worse than it currently is.

@ColinDKelley ColinDKelley changed the title listen uses Time.now for time intervals; should use Monotonic tick counter listen uses Time.now for time intervals; should use Monotonic tick count Nov 9, 2020
ColinDKelley added a commit to Invoca/listen that referenced this issue Nov 9, 2020
ColinDKelley added a commit to Invoca/listen that referenced this issue Nov 9, 2020
@ColinDKelley
Copy link
Collaborator Author

@ioquatix

You can ignore any version that doesn't support it, they should all be EOL.

From reading the documentation, it seems like those constants are more OS-specific than Ruby-version-specific.

So just in case, I wrote the code to be adaptive and prefer Process::CLOCK_MONOTONIC, then Process::CLOCK_MONOTONIC_RAW, and finally to fall back to Time.now.to_f.

@ioquatix
Copy link
Member

ioquatix commented Nov 9, 2020

I have not seen a platform where Process::CLOCK_MONOTONIC is not supported. But if you think it's for the best to be compatible, it's acceptable.

ColinDKelley added a commit to Invoca/listen that referenced this issue Nov 9, 2020
@ColinDKelley ColinDKelley added this to the 3.4 milestone Nov 23, 2020
@ColinDKelley ColinDKelley linked a pull request Nov 23, 2020 that will close this issue
ColinDKelley added a commit to Invoca/listen that referenced this issue Dec 31, 2020
ColinDKelley added a commit to Invoca/listen that referenced this issue Dec 31, 2020
ColinDKelley added a commit to Invoca/listen that referenced this issue Dec 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants