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

bpo-41833: threading.Thread now uses the target name #22357

Merged
merged 3 commits into from
Sep 23, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions Doc/library/threading.rst
Original file line number Diff line number Diff line change
Expand Up @@ -264,8 +264,10 @@ since it is impossible to detect the termination of alien threads.
*target* is the callable object to be invoked by the :meth:`run` method.
Defaults to ``None``, meaning nothing is called.

*name* is the thread name. By default, a unique name is constructed of the
form "Thread-*N*" where *N* is a small decimal number.
*name* is the thread name. By default, a unique name is constructed
of the form "Thread-*N*" where *N* is a small decimal number,
or "Thread-*N* (target)" where "target" is ``target.__name__`` if the
*target* argument is specified.

*args* is the argument tuple for the target invocation. Defaults to ``()``.

Expand All @@ -280,6 +282,9 @@ since it is impossible to detect the termination of alien threads.
base class constructor (``Thread.__init__()``) before doing anything else to
the thread.

.. versionchanged:: 3.10
Use the *target* name if *name* argument is omitted.

.. versionchanged:: 3.3
Added the *daemon* argument.

Expand Down
34 changes: 31 additions & 3 deletions Lib/test/test_threading.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import signal
import textwrap

from unittest import mock
from test import lock_tests
from test import support

Expand Down Expand Up @@ -86,6 +87,33 @@ def tearDown(self):

class ThreadTests(BaseTestCase):

@cpython_only
vstinner marked this conversation as resolved.
Show resolved Hide resolved
def test_name(self):
def func(): pass

thread = threading.Thread(name="myname1")
self.assertEqual(thread.name, "myname1")

# Convert int name to str
thread = threading.Thread(name=123)
self.assertEqual(thread.name, "123")

# target name is ignored if name is specified
thread = threading.Thread(target=func, name="myname2")
self.assertEqual(thread.name, "myname2")

with mock.patch.object(threading, '_counter', return_value=2):
thread = threading.Thread(name="")
self.assertEqual(thread.name, "Thread-2")

with mock.patch.object(threading, '_counter', return_value=3):
thread = threading.Thread()
self.assertEqual(thread.name, "Thread-3")

with mock.patch.object(threading, '_counter', return_value=5):
thread = threading.Thread(target=func)
self.assertEqual(thread.name, "Thread-5 (func)")

# Create a bunch of threads, let each do some work, wait until all are
# done.
def test_various_ops(self):
Expand Down Expand Up @@ -531,7 +559,7 @@ def test_main_thread_after_fork_from_nonmain_thread(self):
import os, threading, sys
from test import support
def f():
def func():
pid = os.fork()
if pid == 0:
main = threading.main_thread()
Expand All @@ -544,14 +572,14 @@ def f():
else:
support.wait_process(pid, exitcode=0)
th = threading.Thread(target=f)
th = threading.Thread(target=func)
th.start()
th.join()
"""
_, out, err = assert_python_ok("-c", code)
data = out.decode().replace('\r', '')
self.assertEqual(err, b"")
self.assertEqual(data, "Thread-1\nTrue\nTrue\n")
self.assertEqual(data, "Thread-1 (func)\nTrue\nTrue\n")

def test_main_thread_during_shutdown(self):
# bpo-31516: current_thread() should still point to the main thread
Expand Down
21 changes: 16 additions & 5 deletions Lib/threading.py
Original file line number Diff line number Diff line change
Expand Up @@ -745,10 +745,9 @@ class BrokenBarrierError(RuntimeError):


# Helper to generate new thread names
_counter = _count().__next__
_counter() # Consume 0 so first non-main thread has id 1.
def _newname(template="Thread-%d"):
return template % _counter()
_counter = _count(1).__next__
def _newname(name_template):
return name_template % _counter()

# Active thread administration
_active_limbo_lock = _allocate_lock()
Expand Down Expand Up @@ -800,8 +799,20 @@ class is implemented.
assert group is None, "group argument must be None for now"
if kwargs is None:
kwargs = {}
if name:
name = str(name)
if not name:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if not name:
else:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to avoid empty thread name, and so I check that the result of str(name) is non-empty. Do you think that it's not worth it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current code allows to set an empty name, and with else the code is simpler. But it does not matter.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Serhiy's suggestion, if someone uses an empty string it was probably intentional. I don't think it's worth trying to catch it.

name_template = "Thread-%d"
if target is not None:
try:
target_name = target.__name__
name_template = f"{name_template} ({target_name})"
except AttributeError:
pass
name = _newname(name_template)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will fail if target.__name__ contains % (please add a test for such case, and include also { just for the case).

It would be safer to write:

name = _newname()
if target is not None:
    ...
    name = f"{name} ({target_name})"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote the code differently to avoid formatting errors.


self._target = target
self._name = str(name or _newname())
self._name = name
self._args = args
self._kwargs = kwargs
if daemon is not None:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
The :class:`threading.Thread` constructor now uses the target name if the
*target* argument is specified but the *name* argument is omitted.