Raymond Hettinger <raymond.hettin...@gmail.com> added the comment:

> Cancelling an unintended event instead of the one we wanted is a bug,
> and prevents me from using the library at all

That problem is certainly worth fixing even if we have to break a few eggs to 
do it (this module has been around for a very long time and almost certainly 
every little detail is being relied on by some users).

The docstring for enter() and enterabs() only promises that it returns an ID 
for an event and that the ID can be used to remove the event.

The regular docs for enter() and enterabs() make a stronger promise that they 
return an "event" but doesn't specify what that means.

The Event class name doesn't has a leading underscore but is not listed in 
__all__ and is not in the main docs.

The *queue* property is documented to return an ordered list of upcoming events 
which are defined to be named tuples with fields for: time, priority, action, 
arguments, kwargs.  That is the most specific and restrictive of the documented 
promises, one that presumably is being relied on in a myriad of ways (a repr 
suitable for logging, a pretty printable list, a sortable list, named access to 
fields, unpackable tuples, etc).

If we respect those published promises, one way to go is to add a unique 
identifier field to the Event class.  If the that unique id is consecutively 
numbered, it would also allow us to guarantee FIFO ordering (not sure whether 
that is actually need though).

Here's a preliminary sketch:

   scheduler.__init__():
+      self.event_sequence = 0   # updated as events are created
        ...

   scheduler.enterabs():
-       event = Event(time, priority, action, argument, kwargs)
+       self.event_sequence += 1
+       event = Event(time, priority, self.event_sequence, action, argument, 
kwargs)

- class Event(namedtuple('Event', 'time, priority, action, argument, kwargs')):
-    __slots__ = []
-    def __eq__(s, o): return (s.time, s.priority) == (o.time, o.priority)
-    def __lt__(s, o): return (s.time, s.priority) <  (o.time, o.priority)
-    def __le__(s, o): return (s.time, s.priority) <= (o.time, o.priority)
-    def __gt__(s, o): return (s.time, s.priority) >  (o.time, o.priority)
-    def __ge__(s, o): return (s.time, s.priority) >= (o.time, o.priority)
+ Event = namedtuple('Event', 'time, priority, sequence, action, argument, 
kwargs')

The user visible effects are:

* A new field would be displayed.
* Any code that constructs Event objects manually would break instantly.
* Any code using tuple unpacking on Event objects would break instantly.
* Slightly slower Event creation time.
* Faster heap updates (because native tuple ordering is faster than pure python 
rich comparisons).

However, outside the mention in the documentation for the *queue* property, 
Event objects are not part of the public API.  Also, we do have precedent for 
adding new fields without a negative consequence (argument and kwargs were 
added in Python 3.3).

An alternative solution for removing exact events (not for FIFO stability) is 
to have enter() and enterabs() return an opaque, unique event identifier.  Then 
the code for cancel() would need to be changed to something like:

    self._queue = [e for e in self._queue if id(e) != event]
    heapq.heapify(self._queue)

The user visible effects are:

* enter() and enterabs() no longer return an event object
  (it was implied that they would, but not explictly promised).
  Any code relying on an actual Event object being returned
  would break.
* cancel() would only accept the opaque IDs
  (again, it was only implied that actual Event objects would be used).
  Code would break that took Event objects out of the *queue* listing
  and passed those into cancel().
* cancel() would run a bit faster (because calling id() is cheaper
  that calling the pure python rich comparisons and having them
  extract the time and priority fields).

Either way breaks a few eggs but makes the module safer to use.

----------

_______________________________________
Python tracker <rep...@bugs.python.org>
<https://bugs.python.org/issue19270>
_______________________________________
_______________________________________________
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

Reply via email to