Add optional event debouncing for auto-restart

[taleinat]

If a debounce interval is specified, upon receiving events, collect events until the interval passes with no incoming events, and only then trigger a restart.

Closes #542.

Note to reviewers: The more involved start and stop mechanisms in AutoRestartTrick are needed to avoid restarting the debouncer when restart the process, and to ensure that the debouncer is properly stopped when stopping the trick.

[BoboTiG]

Thanks a lot @taleinat!

Would you mind adding a line in the changelog too, please?

[taleinat]

Would you mind adding a line in the changelog too, please?

Of course, done.

[oprypin]

Why do we need to ensure that it runs only once? It seems like it should behave OK even if called multiple times.

[oprypin]

Here's a different implementation, like I did it in MkDocs.

It has the benefit of being instantly stoppable, rather than having a 0 - 0.2 sec delay before being able to stop.

It also ensures to instantly stop firing events when it's stopped, maybe eliminating the need for

The more involved start and stop mechanisms in AutoRestartTrick are needed to avoid restarting the debouncer when restart the process

Suggested change

	import logging
	import queue
	from watchdog.utils import BaseThread
	logger = logging.getLogger(__name__)
	class EventDebouncer(BaseThread):
	"""Background thread for debouncing event handling.
	When an event is received, wait until the configured debounce interval
	passes before calling the callback. If additional events are received
	before the interval passes, reset the timer and keep waiting. When the
	debouncing interval passes, the callback will be called with a list of
	events in the order in which they were received.
	"""
	def __init__(self, debounce_interval_seconds, events_callback):
	super().__init__()
	self.debounce_interval_seconds = debounce_interval_seconds
	self.events_callback = events_callback
	self._event_queue = queue.Queue()
	def handle_event(self, event):
	self._event_queue.put(event)
	def run(self):
	while self.should_keep_running():
	events = []
	# Get first event.
	try:
	events.append(self._event_queue.get(timeout=0.2))
	except queue.Empty:
	continue
	if self.debounce_interval_seconds:
	# Collect additional events until the debounce interval passes.
	while self.should_keep_running():
	try:
	events.append(self._event_queue.get(timeout=self.debounce_interval_seconds))
	except queue.Empty:
	break
	self.events_callback(events)
	import logging
	import threading
	logger = logging.getLogger(__name__)
	class EventDebouncer(threading.Thread):
	"""Background thread for debouncing event handling.
	When an event is received, wait until the configured debounce interval
	passes before calling the callback. If additional events are received
	before the interval passes, reset the timer and keep waiting. When the
	debouncing interval passes, the callback will be called with a list of
	events in the order in which they were received.
	"""
	def __init__(self, debounce_interval_seconds, events_callback):
	super().__init__()
	self.debounce_interval_seconds = debounce_interval_seconds
	self.events_callback = events_callback
	self._cond = threading.Condition()
	self._shutdown = False
	self._events = []
	def handle_event(self, event):
	with self._cond:
	self._events.append(event)
	self._cond.notify_all()
	def run(self):
	while True:
	with self._cond:
	self._cond.wait_for(lambda: self._events or self._shutdown)
	if self._shutdown:
	break
	if self.debounce_interval_seconds:
	while self._cond.wait(timeout=self.debounce_interval_seconds):
	# Wait for events to stop happening.
	pass
	events = list(self._events)
	self._events.clear()
	self.events_callback(events)
	def stop(self):
	with self._cond:
	self._shutdown = True
	self._cond.notify_all()

[taleinat]

Thanks for this @oprypin!

Using a condition like this is indeed better for instant stopping, I'll work that into the implementation.

[oprypin]

@taleinat Well you can just "accept suggestion", I think that will also end up simpler than, for example, keeping it a queue. It's a fully functioning replacement, just in 1 of the new tests will need to add a microscopic sleep to make it pass.

[taleinat]

Thanks, and your work to make it such a drop-in replacement was helpful and is appreciated!

This version however does not handle well the case where a shutdown signal is given during the debouncing interval. Also, the use of Condition.wait_for() is more complex than necessary.

So instead I've integrated the approach while addressing these two concerns.

[BoboTiG]

I'm wondering if we can get the debouncer for watchdog too 🤔
Did you already think about such thing @taleinat?

[taleinat]

I'm wondering if we can get the debouncer for watchdog too 🤔

@BoboTiG could you explain what you mean by that? This is already implemented as a reusable EventDebouncer class.

Do you mean whether this could be made easier to use with Trick sub-classes, e.g. with AutoRestartTrick?

[oprypin]

@taleinat I'm guessing what was meant (or at least I think it would be good) if usages of Observer / observer.schedule could simply add a debounce parameter without needing to use a separate class. But you're right, your class can indeed satisfy this requirement. And maybe it's already the most idiomatic usage, but in that case an example of how to use it with an observer and your own callback would really make your idea shine.

[BoboTiG]

@taleinat sorry for thr lack of information :)
The idea from @oprypin is what I meant: introduce an easy way for users to use the new class. The keyword argument seems like a good idea.

[taleinat]

@taleinat sorry for thr lack of information :) The idea from @oprypin is what I meant: introduce an easy way for users to use the new class. The keyword argument seems like a good idea.

I spent several hours looking into this, but I haven't found a reasonable way to achieve this with the current design of the watchdog codebase.

EventEmitter classes can emit many different types of events. Supporting the large number of possible combinations of which events to debounce, together or separately, seems like it would make things harder and more complex than using the EventDebouncer class directly. The same goes for EventDispatcher.

I think auto-restart is a bit of a special use-case, as it treats all events from all files identically. When implementing this, I did consider whether it would be good to also implement debouncing for other commands (especially shell-command), but I couldn't think of good use cases for that.

So, to summarize, IMO this change is immediately useful and the implementation is reasonable, so it should go in. Future improvements may be done in the future ;)

[BoboTiG]

You made a point. Let's merge that great PR when I'll find some time on a real computer :)