Title: threading.Timer.__init__() should use immutable argument defaults for args and kwargs
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.4, Python 3.3
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: denversc, gvanrossum, loewis, python-dev, r.david.murray, terry.reedy
Priority: normal Keywords: patch

Created on 2013-03-16 00:17 by denversc, last changed 2013-03-30 21:25 by r.david.murray. This issue is now closed.

File name Uploaded Description Edit
ThreadingTimerInitDefaultArgsIssueDemo.01.patch denversc, 2013-03-16 00:17 C:\dev\cpython\ThreadingTimerInitDefaultArgsIssueDemo.01.patch review
Messages (9)
msg184278 - (view) Author: Denver Coneybeare (denversc) * Date: 2013-03-16 00:17
The __init__() method of threading.Timer uses *mutable* default values for the "args" and "kwargs" arguments.  Since the default argument objects are created once and re-used for each instance, this means that changing the args list or kwargs dict of a Timer object that used the argument defaults will specify those arguments to all future Timer objects that use the defaults too.

def __init__(self, interval, function, args=[], kwargs={}):

A fully backwards-compatible way to fix this is to instead use None as the default value for args and kwargs and just create a new list and/or dict inside __init__() if they are None.  That way each new instance of Timer will get its very own args list and kwargs dict object.

def __init__(self, interval, function, args=None, kwargs=None):
    self.args = args if args is not None else []
    self.kwargs = kwargs if kwargs is not None else {}

Here is a sample script that reproduces the issue:

    import threading

    event = threading.Event()
    def func(*args, **kwargs):
        print("args={!r} kwargs={!r}".format(args, kwargs))

    timer1 = threading.Timer(1, func)
    timer1.kwargs["foo"] = "bar"

    timer2 = threading.Timer(1, func)

Here is the example output when run before the fix:

args=('blah',) kwargs={'foo': 'bar'}
[44758 refs, 17198 blocks]

And after the fix:

args=() kwargs={}
[47189 refs, 18460 blocks]

As you can see, in the version without the fix, the elements added to timer1's args and kwargs were also given to timer2, which is almost certainly not what a user would expect.

A proposed patch, ThreadingTimerInitDefaultArgsIssueDemo.01.patch, is attached.  This fixes the issue, updates the docs, and adds a unit test.
msg184282 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-03-16 01:42
Hmm.  This wasn't an issue before 3.3 because previously one couldn't subclass Timer.  So yeah, this needs to be fixed, but only in 3.3 and tip.

Thanks for the patch.
msg184335 - (view) Author: Denver Coneybeare (denversc) * Date: 2013-03-16 17:15
Thanks r.david.murray for your feedback.  Although I disagree with your conclusion that this does not affect 2.7.  Just try running the "sample script that reproduces the issue" from my first post and you will see the erroneous behaviour in 2.7.  Even though threading.Timer is a function in 2.7 (instead of a class), it still ultimately returns a class whose args and kwargs members can be modified.
msg184340 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-03-16 17:57
I'm sorry, you are correct.  I replied too quickly without thinking it through.
msg184762 - (view) Author: Denver Coneybeare (denversc) * Date: 2013-03-20 13:30
Thanks r.david.murray.  I appreciate you taking the time to look at this issue!
msg185400 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2013-03-28 02:14
The reported behavior is not a bug by our usual standards. The code is exactly as documented.
manual: class threading.Timer(interval, function, args=[], kwargs={})
docstring: t = Timer(30.0, f, args=[], kwargs={})

Threading is not a beginner module. Any competent Python programmer who reads either of the above, or the code line you quoted, would expect exactly the behavior you report. I think we should presume that people who monkey-patch the class, which is an unusual thing to do, know what they are doing. The patch would break any such intentional usage. If the signature were to be changed, there should be a deprecation period first, preferably with a DeprecationWarning for mutation either through an instance or through .__init__.

I do not see anything special about this particular function. If we change this use of [] and {} as defaults, then we should look at all such uses in the stdlib -- after pydev discussion. But I currently think we should leave well enough alone.

The Timer class was added in Sept. 2001, rev 19727, issue #428326. The patch was written by Itamar Shtull-Trauring, approved by Guido, and reviewed and committed by Martin.
msg185401 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2013-03-28 03:16
I agree with the OP -- it's a simple fix and the current code definitely violates our recommendations.  I see no reason not to submit this (if there's nothing *else* wrong with it -- it actually seems pretty complete).  Not sure how important it is to fix in 2.7, but I don't see a problem with it either.
msg185591 - (view) Author: Roundup Robot (python-dev) Date: 2013-03-30 21:23
New changeset 2698920eadcd by R David Murray in branch '3.3':
Issue #17435: Don't use mutable default values in Timer.

New changeset 8c15e57830dd by R David Murray in branch 'default':
Merge #17435: Don't use mutable default values in Timer.
msg185592 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-03-30 21:25
Thanks, Denver.

I'm choosing not to backport it to 2.7, but that can be done later if someone finds it worth doing.
Date User Action Args
2013-03-30 21:25:55r.david.murraysetstatus: open -> closed
versions: - Python 2.7, Python 3.2
messages: + msg185592

resolution: fixed
stage: patch review -> resolved
2013-03-30 21:23:21python-devsetnosy: + python-dev
messages: + msg185591
2013-03-28 03:16:37gvanrossumsetmessages: + msg185401
2013-03-28 02:14:22terry.reedysetnosy: + loewis, terry.reedy, gvanrossum
messages: + msg185400
2013-03-20 13:30:07denverscsetmessages: + msg184762
2013-03-16 17:57:37r.david.murraysetmessages: + msg184340
versions: + Python 2.7, Python 3.2
2013-03-16 17:15:02denverscsetmessages: + msg184335
2013-03-16 01:42:08r.david.murraysetversions: - Python 3.1, Python 2.7, Python 3.2
nosy: + r.david.murray

messages: + msg184282

stage: patch review
2013-03-16 00:17:20denversccreate