[issue19270] Document that sched.cancel() doesn't distinguish equal events and can break order

[Andrei Kulakov]

Andrei Kulakov  added the comment:

This can be closed as fixed.

--
nosy: +andrei.avk

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19270] Document that sched.cancel() doesn't distinguish equal events and can break order

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:


New changeset 5368c2b6e23660cbce7e38dc68f859c66ac349ee by Bar Harel in branch 
'master':
bpo-19270: Fixed sched.scheduler.cancel to cancel correct event (GH-22729)
https://github.com/python/cpython/commit/5368c2b6e23660cbce7e38dc68f859c66ac349ee


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19270] Document that sched.cancel() doesn't distinguish equal events and can break order

[Bar Harel]

Change by Bar Harel :


--
pull_requests: +21694
stage: needs patch -> patch review
pull_request: https://github.com/python/cpython/pull/22729

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19270] Document that sched.cancel() doesn't distinguish equal events and can break order

[Jonas Norling]

Jonas Norling  added the comment:

@bar.harel: I didn't find a PR, so I'd like to encourage you to submit one :-)

I stumbled onto this bug when the scheduler would cancel the wrong event for me 
(Python 3.7, 3.8). Raymond's suggestion 1 sounds reasonable; it would be very 
unlikely to break code that doesn't depend on internals in sched, and it 
simplifies the implementation a bit.

--
nosy: +wocket

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19270] Document that sched.cancel() doesn't distinguish equal events and can break order

[Bar Harel]

Bar Harel  added the comment:

@Raymond your first idea sounds good and was the first thing that came to my 
mind.
I only worried about breaking things, so I gave the more conservative 
suggestion.

If breaking a few eggs isn't an issue and the implications of your idea are 
agreed upon, I'll patch and add a PR.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19270] Document that sched.cancel() doesn't distinguish equal events and can break order

[Raymond Hettinger]

Raymond Hettinger  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 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19270] Document that sched.cancel() doesn't distinguish equal events and can break order

[STINNER Victor]

Change by STINNER Victor :


--
nosy:  -vstinner

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19270] Document that sched.cancel() doesn't distinguish equal events and can break order

[Bar Harel]

Bar Harel  added the comment:

I've just encountered this bug as well.

Although this issue is old, #1 should be fixed whether we like it or not. 
Cancelling an unintended event instead of the one we wanted is a bug, and 
prevents me from using the library at all (I schedule events based on absolute 
time).

To be honest, I do not believe anyone uses scheduler.cancel(Event(time, 
priority, ...)) in order to cancel an event based on the given time and 
priority.

Even if they do so, Event itself is undocumented and is being treated mostly as 
an ID for a currently scheduled event.

A good solution would be to add a DeprecationWarning to the ordering functions, 
so people won't use them, and remove __eq__ at 3.12.

The only issue that will arise is that an event1 might be >= than event2, <= 
than event 2, and != to event 2, as the other ordering will stay. We can fix 
this by implementing a slower lookup but I believe it isn't a real issue.

--
nosy: +bar.harel

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19270] Document that sched.cancel() doesn't distinguish equal events and can break order

[Josh Rosenberg]

Josh Rosenberg  added the comment:

Victor: "I would be interested of the same test on Windows."

Looks like someone performed it by accident, and filed #34943 in response 
(because time.monotonic() did in fact return the exact same time twice in a row 
on Windows).

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19270] Document that sched.cancel() doesn't distinguish equal events and can break order

[Josh Rosenberg]

Josh Rosenberg added the comment:

sched has been around for a long time, but it's been useless for so many 
purposes that it *should* handle (completely unsafe in threaded contexts until 
3.3, still can't handle useful threaded scenarios today, e.g. scheduling tasks 
for short delays when draining the task queue, waiting on a task with a long 
delay, see #20126 ) that calling it acceptable is more about lack of available 
uses than acceptable design.

Saying don't schedule two events for the same time and priority if expect to 
cancel either of them is not a reasonable solution; the main reason to 
schedule two such events in that way would be to have the option to cancel one 
but not the other; as is, trying to cancel one will (pseudo-)randomly cancel 
either of them. I don't particularly care how it's fixed (though the proposed 
fix for #13451 seems like a good one), and the issue with changing event order 
isn't so important, but cancelling the wrong event is really bad.

--
nosy: +josh.r

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19270
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19270] Document that sched.cancel() doesn't distinguish equal events and can break order

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

Well. In any case I have no ideas how fix it. Any behavior change will break 
existing code.

I requalify this issue as documentation enhancement. The documentation should 
specify that evens scheduled on same tame with same priority considered equal. 
That cancel() doesn't distinguish equal events and can cancel arbitrary of 
them. That cancel() doesn't preserve order of equal events.

--
assignee:  - docs@python
components: +Documentation -Library (Lib)
keywords: +easy
nosy: +docs@python
title: sched.cancel() breaks events order - Document that sched.cancel() 
doesn't distinguish equal events and can break order
type: behavior - enhancement
versions: +Python 2.7, Python 3.3

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19270
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19270] Document that sched.cancel() doesn't distinguish equal events and can break order

[STINNER Victor]

STINNER Victor added the comment:

@Raymond: The logic is essentially the same as is used in popular event loops 
like Tornado.

Sorry, I didn't understand your sentence: do you mean that sched must keep the 
insertion order or the order is undefined?

Undefined order is fine if it's documented.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19270
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19270] Document that sched.cancel() doesn't distinguish equal events and can break order

[Antoine Pitrou]

Antoine Pitrou added the comment:

I'm not strong on a new invariant, however I think bug #1 would deserve fixing:

 1. sched.cancel() can remove wrong event (because it uses equality instead 
 identity).

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19270
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19270] Document that sched.cancel() doesn't distinguish equal events and can break order

[Serhiy Storchaka]

Serhiy Storchaka added the comment:

 I'm not strong on a new invariant, however I think bug #1 would deserve 
 fixing:

How would you do this?

1. Use identity instead equality to search canceled event. It will break code 
where an user cancels an event by time and priority: 
scheduler.cancel(Event(time, priority, ...)).

2. Always cancel chronologically last (first) of equals events. This requires 
popp-ing and push-ing all events. Too slow.

3. Add an ordered number to the event. This will slow down all too.

4. Mixed strategy. First use identity search, then equality search, and only if 
found several equals events fallback to slow variant. This is too complicated. 
It will work as expected in most normal cases, but in rare cases... This 
behavior would hard to document and understand.

If you have better idea, patch is welcome.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19270
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19270] Document that sched.cancel() doesn't distinguish equal events and can break order

[Antoine Pitrou]

Antoine Pitrou added the comment:

 4. Mixed strategy. First use identity search, then equality search,
 and only if found several equals events fallback to slow variant. This
 is too complicated. It will work as expected in most normal cases, but
 in rare cases... This behavior would hard to document and understand.

You're right. So perhaps this should simply be documented as is.

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19270
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue19270] Document that sched.cancel() doesn't distinguish equal events and can break order

[Giampaolo Rodola']

Giampaolo Rodola' added the comment:

 This module has been around for a long time and no users have reported 
 any issues with respect to event ordering.  The logic is essentially 
 the same as is used in popular event loops like Tornado.
 Please don't just make-up a new invariant for this module.

+1

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue19270
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com