GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-41833: threading.Thread now uses the target name by vstinner · Pull Request #22357 · python/cpython

Conversation

vstinner commented Sep 22, 2020 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

https://bugs.python.org/issue41833

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

the-knights-who-say-ni added the CLA signed label

Sep 22, 2020

bedevere-bot added the awaiting core review label

Sep 22, 2020

vstinner commented Sep 22, 2020

Copy link

Copy Markdown

Member Author

cc @pablogsal @methane @pitrou @serhiy-storchaka

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

pablogsal approved these changes

Sep 22, 2020

View reviewed changes

pablogsal left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

I left a minor nit

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Doc/library/threading.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

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 Resolved

Uh oh!

There was an error while loading. Please reload this page.

vstinner commented Sep 22, 2020

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


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

8b4397d

vstinner commented Sep 23, 2020

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka reviewed

Sep 23, 2020

View reviewed changes

Comment thread

Lib/threading.py Outdated

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

serhiy-storchaka Sep 23, 2020

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Sep 23, 2020

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Sep 23, 2020

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ammaraskar Sep 23, 2020

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/threading.py Outdated

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

serhiy-storchaka Sep 23, 2020

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

vstinner Sep 23, 2020

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Avoid formatting errors

51b15fb

vstinner commented Sep 23, 2020

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka approved these changes

Sep 23, 2020

View reviewed changes

Comment thread

Lib/threading.py Outdated

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

serhiy-storchaka Sep 23, 2020

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka commented Sep 23, 2020

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ammaraskar approved these changes

Sep 23, 2020

View reviewed changes

ammaraskar left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_threading.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Lib/threading.py Outdated

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

ammaraskar Sep 23, 2020

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          Allow to set the name to an empty string

859b4c2

vstinner commented Sep 23, 2020

Copy link

Copy Markdown

Member Author

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).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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

vstinner commented Sep 23, 2020

Copy link

Copy Markdown

Member Author

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 ;-)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

shihai1991 added a commit to shihai1991/cpython that referenced this pull request

Sep 24, 2020


          Merge remote-tracking branch 'origin/master' into bpo_39337_new_3

e29ec44

* 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)
  ...

shihai1991 added a commit to shihai1991/cpython that referenced this pull request

Sep 29, 2020


          Merge remote-tracking branch 'origin/master' into bpo_1635741_1

e3b9365

* 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)
  ...

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

Reviewers

ammaraskar

ammaraskar approved these changes

pablogsal

pablogsal approved these changes

serhiy-storchaka

serhiy-storchaka approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vstinner commented Sep 22, 2020 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.