bpo-41833: threading.Thread now uses the target name by vstinner · Pull Request #22357 · python/cpython · GitHub

https://bugs.python.org/issue41833

cc @pablogsal @methane @pitrou @serhiy-storchaka

LGTM

I left a minor nit

@serhiy-storchaka raised some concerns in https://bugs.python.org/issue41833

I rewrote my PR to generate names like "Thread-3 (func)", rather than just "func". So if two target functions have the same name, or if two threads use the same target, it remains possible to distinguish the two threads.

cc @serhiy-storchaka

Suggested change

      
                    if not name:
          
                    else:

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?

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

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.

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})"

I wrote the code differently to avoid formatting errors.

@ammaraskar @pablogsal: Would you mind to review the updated PR?

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

I am wondering in what case you would specify a non-string (and non-None) name.

I am wondering in what case you would specify a non-string (and non-None) name.

Probably a numeric id? Still, it's probably better to maintain the old behavior.

LGTM with one small question around the test.

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.

Ok, I changed the code to use else:.

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.

In Python 3.9, threading.Thread(name='').name returns 'Thread-1'. My PR didn't change that and still doesn't change that.

I tried to change the behavior for such special case:

import threading

class Name:
    def __nonzero__(self):
        return True
    def __str__(self):
        return ''

name = threading.Thread(name=Name()).name
print(repr(name))

name is an empty string before and after my change (with my latest commit).

Thanks for reviews and help to decide what is the correct way to name threads ;-) I merged my PR.

If someone wants to adjust the behavior for bool(name) being false, or empty name string, you can write a new PR ;-)

* origin/master: (27 commits) bpo-41428: Fix compiler warnings in unionobject.c (pythonGH-22388) bpo-41654: Fix compiler warning in MemoryError_dealloc() (pythonGH-22387) bpo-41833: threading.Thread now uses the target name (pythonGH-22357) bpo-30155: Add macros to get tzinfo from datetime instances (pythonGH-21633) bpo-33822: Update IDLE section of What's New 3.8 (pythonGH-22383) bpo-41844: Add IDLE section to What's New 3.9 (GN-22382) bpo-41841: Prepare IDLE News for 3.10 (pythonGH-22379) bpo-37779 : Add information about the overriding behavior of ConfigParser.read (pythonGH-15177) bpo-40170: Use inline _PyType_HasFeature() function (pythonGH-22375) bpo-40941: Fix stackdepth compiler warnings (pythonGH-22377) bpo-40941: Fix fold_tuple_on_constants() compiler warnings (pythonGH-22378) bpo-40521: Fix PyUnicode_InternInPlace() (pythonGH-22376) bpo-41834: Remove _Py_CheckRecursionLimit variable (pythonGH-22359) bpo-1635741, unicodedata: add ucd_type parameter to UCD_Check() macro (pythonGH-22328) bpo-1635741: Port _lsprof extension to multi-phase init (PEP 489) (pythonGH-22220) bpo-41513: Improve order of adding fractional values. Improve variable names. (pythonGH-22368) bpo-41816: `StrEnum.__str__` is `str.__str__` (pythonGH-22362) bpo-35764: Rewrite the IDLE Calltips doc section (pythonGH-22363) bpo-41810: Reintroduce `types.EllipsisType`, `.NoneType` & `.NotImplementedType` (pythonGH-22336) bpo-41602: raise SIGINT exit code on KeyboardInterrupt from pymain_run_module (python#21956) ...

* origin/master: (113 commits) bpo-41773: Raise exception for non-finite weights in random.choices(). (pythonGH-22441) bpo-41873: Add vectorcall for float() (pythonGH-22432) bpo-41861: Convert _sqlite3 PrepareProtocolType to heap type (pythonGH-22428) bpo-41842: Add codecs.unregister() function (pythonGH-22360) bpo-41875: Use __builtin_unreachable when possible (pythonGH-22433) bpo-40105: ZipFile truncate in append mode with shorter comment (pythonGH-19337) bpo-41870: Use PEP 590 vectorcall to speed up bool() (pythonGH-22427) [doc] Leverage the fact that the actual types can now be indexed for typing (pythonGH-22340) bpo-41861: Convert _sqlite3 cache and node static types to heap types (pythonGH-22417) bpo-41858: Clarify line in optparse doc (pythonGH-22407) Revert "Fix all Python Cookbook links (python#22205)" (pythonGH-22424) bpo-1635741: Port _bisect module to multi-phase init (pythonGH-22415) bpo-41428: Fix compiler warning in unionobject.c (pythonGH-22416) Fix logging error message (pythonGH-22410) bpo-39934: Account for control blocks in 'except' in compiler. (pythonGH-22395) bpo-41775: Make 'IDLE Shell' the shell title (python#22399) bpo-41428: Fix compiler warnings in unionobject.c (pythonGH-22388) bpo-41654: Fix compiler warning in MemoryError_dealloc() (pythonGH-22387) bpo-41833: threading.Thread now uses the target name (pythonGH-22357) bpo-30155: Add macros to get tzinfo from datetime instances (pythonGH-21633) ...

the-knights-who-say-ni added the CLA signed label Sep 22, 2020

bedevere-bot added the awaiting core review label Sep 22, 2020

pablogsal approved these changes Sep 22, 2020

View reviewed changes

Comment thread Doc/library/threading.rst Outdated

bedevere-bot added awaiting merge and removed awaiting core review labels Sep 22, 2020

pablogsal reviewed Sep 22, 2020

View reviewed changes

Comment thread Doc/library/threading.rst Outdated

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

8b4397d

serhiy-storchaka reviewed Sep 23, 2020

View reviewed changes

Avoid formatting errors

51b15fb

serhiy-storchaka approved these changes Sep 23, 2020

View reviewed changes

ammaraskar approved these changes Sep 23, 2020

View reviewed changes

Allow to set the name to an empty string

859b4c2

vstinner merged commit 98c16c9 into python:master Sep 23, 2020

bedevere-bot removed the awaiting merge label Sep 23, 2020

vstinner deleted the thread_name branch September 23, 2020 21:21

xzy3 pushed a commit to xzy3/cpython that referenced this pull request Oct 18, 2020

bpo-41833: threading.Thread now uses the target name (pythonGH-22357 )

ef5e98e

                           kwargs = {}
+                      if name:
+                          name = str(name)
+                      if not name:

+                                  name_template = f"{name_template} ({target_name})"
+                              except AttributeError:
+                                  pass
+                          name = _newname(name_template)

                           kwargs = {}
+                      if name:
+                          name = str(name)
+                      if not name:

                           kwargs = {}
+                      if name:
+                          name = str(name)
+                      if not name:

bpo-41833: threading.Thread now uses the target name by vstinner · Pull Request #22357 · python/cpython · GitHub

Uh oh!

Uh oh!

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

bpo-41833: threading.Thread now uses the target name#22357vstinner merged 3 commits intopython:masterpython/cpython:masterfrom vstinner:thread_nameCopy head branch name to clipboard

Conversation

vstinner commented Sep 22, 2020 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Sep 22, 2020

Uh oh!

pablogsal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vstinner commented Sep 22, 2020

Uh oh!

vstinner commented Sep 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Sep 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Sep 23, 2020

Uh oh!

ammaraskar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Sep 23, 2020

Uh oh!

vstinner commented Sep 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bpo-41833: threading.Thread now uses the target name#22357
vstinner merged 3 commits into
python:masterfrom
vstinner:thread_name

vstinner commented Sep 22, 2020 •
edited by bedevere-bot
Loading