bpo-41149 Fix a bug in threading that causes threads to fail to start.

[BarneyStratford]

>>> import threading
>>> class foo (object):
...     def __bool__ (self):
...         return False
...     def __call__ (self):
...         print ("Running")
...
>>> threading.Thread (target = foo ()).start ()

The expected result of these commands would be for the thread to print
"Running". However, in actual fact it prints nothing at all. This is
because threading.Thread.run only runs the target if it is True as a
boolean. This is presumably to make the thread do nothing at all if
the target is None. In this case, I have a legitimate target that is
False as a boolean.

I propose to remove the test altogether. The effect of this is that
failure to set the target of the thread, or setting a non-callable
target, will cause the thread to raise a TypeError as soon as it is
started. Forgetting to set the target is in almost every case a bug,
and bugs should never be silent.

Edit: make the code appear correctly

https://bugs.python.org/issue41149

[the-knights-who-say-ni]

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@BarneyStratford

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

[WildCard65]

@BarneyStratford Pull request title needs to start with: "bpo-41149" (Minus the quotes of course), the 'bpo' part is all lowercase.

[BarneyStratford]

Thanks, @WildCard65. Changed now.

[WildCard65]

@BarneyStratford Now all you need to do is sign that CLA, and then once it's shown that it's signed, do something to get @the-knights-who-say-ni to recheck.

[BarneyStratford]

Already signed it. Should I find an old woman to say "Ni" to, or is there a better way to get the knights going?

[WildCard65]

@BarneyStratford On bugs.python.org there's a button labelled "Your details" for when you're logged in, click it and then fill out the field for "Github Username" (without the '@' symbol), let me know when you've done it.

[BarneyStratford]

Yeah, my username over there had a space in it. I changed it earlier this afternoon.

[WildCard65]

@BarneyStratford Mind actually linking your Github Account to BPO?

[BarneyStratford]

Ah yes. Oopsy.

[remilapeyre]

Hi @BarneyStratford, thanks for your contribution!

For your change to be accepted it will need a test.

Also you should not break the following code as some users may rely on it:

>>> import threading
>>> threading.Thread().start()
Exception in thread Thread-1:
Traceback (most recent call last):
  File "/Users/remi/src/cpython/Lib/threading.py", line 949, in _bootstrap_inner
    self.run()
  File "/Users/remi/src/cpython/Lib/threading.py", line 887, in run
    self._target(*self._args, **self._kwargs)
TypeError: 'NoneType' object is not callable

Please make sure it continues to work by checking for None explicitly.

[BarneyStratford]

I'm not really sure how it can be tested, as a non-callable target throws its exception in the new thread, where it can't be caught.

[remilapeyre]

I'm not really sure how it can be tested, as a non-callable target throws its exception in the new thread, where it can't be caught.

To make sure the target is called when it's boolean value is False you could do:

>>> import threading
>>> e = threading.Event()
>>> class foo:
...     def __bool__(self): return False
...     def __call__(self): e.set()
...
>>> threading.Thread(target=foo()).start()
>>> e.wait(0.1)
True

To make sure it works when target is None, you could do:

>>> e = threading.Event()
>>> class NoneTarget(threading.Thread):
...     def run(self):
...             super().run()
...             e.set()
...
>>> NoneTarget().start()
Exception in thread Thread-2:
Traceback (most recent call last):
  File "/Users/remi/src/cpython/Lib/threading.py", line 949, in _bootstrap_inner
>>>     self.run()
  File "<stdin>", line 3, in run
  File "/Users/remi/src/cpython/Lib/threading.py", line 887, in run
    self._target(*self._args, **self._kwargs)
TypeError: 'NoneType' object is not callable

>>> e.wait(0.1)
False

In both cases the result of e.wait(0.1) should be True.

[BarneyStratford]

Hmm, I seem to have failed to make my meaning clear. The thing I can't see how to test is this:

threading.Thread(target=object()).start()

Since the target isn't callable, we would want to check that the thread doesn't run at all and the TypeError gets raised. However, we can't check what errors are raised inside a thread.

[remilapeyre]

Hmm, I seem to have failed to make my meaning clear. The thing I can't see how to test is this:

threading.Thread(target=object()).start()

Since the target isn't callable, we would want to check that the thread doesn't run at all and the TypeError gets raised. However, we can't check what errors are raised inside a thread.

Sorry, I misunderstood, you can have a look at threading.excepthook():

>>> threading.excepthook = print
>>> threading.Thread(target=object()).start()
_thread.ExceptHookArgs(exc_type=<class 'TypeError'>, exc_value=TypeError("'object' object is not callable"), exc_traceback=<traceback object at 0x7ff3401d6ec0>, thread=<Thread(Thread-4, started 123145372155904)>)

[BarneyStratford]

Ooh, new feature! I like it. I'm on 3.6, which is why I hadn't yet seen it.

In that case, quite a few of the threading tests can be updated. I'll work on another commit, and get back to you.

[remilapeyre]

Ooh, new feature! I like it. I'm on 3.6, which is why I hadn't yet seen it.

Yes! It's super useful!

In that case, quite a few of the threading tests can be updated.

Please don't change the existing tests to use that, it's a laudable intention but it's actually very hard to change the test and be sure not to have broken anything, as there is nothing that tests the tests.

[BarneyStratford]

This pull request is now done. Can it be reviewed and (if appropriate) merged now?

[remilapeyre]

Hi @BarneyStratford, thanks for making the changes! One last thing before making the final review, please make sure your code respect the PEP 8 coding style (https://www.python.org/dev/peps/pep-0008/).

For example it should be def __bool__(self): instead of def __bool__ (self): (notice the absence of whitespace before ( in the first case).

[BarneyStratford]

Done it.

[BarneyStratford]

Anybody still here? The PR is ready to be merged.

[remilapeyre]

Hi @BarneyStratford, sorry I haven't had much time to review this lately.

[remilapeyre]

I think you can replace this whole Test Case by adding

    def test_boolean_conversion(self):
        # bpo-41149: Previously, a thread target that was False as a boolean
        # would not run, regardless of whether it was callable. The correct
        # behaviour is for a thread to do nothing if its target is None, to
        # raise an exception in the thread if the target is non-callable, and to
        # call the target if it is callable.

        class BooleanTarget(threading.Thread):
            ran = False

            def __bool__(self):
                return False

            def __call__(self):
                self.ran = True

        target = BooleanTarget()
        thread = threading.Thread(target=target)
        thread.start()
        thread.join()
        self.assertTrue(target.ran)

at the end of the ThreadTests class. All the other classes like when the __bool__ returns True or if it's not callable are already covered by the current tests.

[remilapeyre]

Suggested change

	Fix a bug that can cause threads to fail to start.
	Fix a bug that can cause threads to fail to start. Patch contributed by Barney Stratford.

[remilapeyre]

Thanks, it looks good, there is just a couple of extra blank lines that can be removed.

[remilapeyre]

Suggested change

[remilapeyre]

Thanks for reporting and fixing this @BarneyStratford ! The change looks good to me, sorry I had not a lot of time to review it.

Now a core developer will need to make a second review and may either approve and merge the change or ask you for some additional modifications. They have a lot to do on Python so it may take them some time to get to this PR but hopefully they will do it :)

[BarneyStratford]

Any chance we can get a decision on this? It's been four months since the last changes now...

[BarneyStratford]

Thanks for getting this sorted!

[pablogsal]

Thanks for getting this sorted!

Thank you for the PR :)