[issue27908] del _limbo[self] KeyError

[Maciej Urbański]

Maciej Urbański added the comment:

To address @Dima.Tisnek concern I have changed exception message in case thread 
start process is merely in progress.

I kept `self._started` check under a lock so we can avoid more extreme race 
condition of one thread checking `self._started` right before another sets it 
and exits the limbo.

As for testing `self._started` under a lock, but setting it without one. I'm 
avoiding it only because of performance reasons. The read is super cheap, while 
notification of `.set()` is more complex, so if aesthetics are only reasons for 
doing it there then I would advise against holding that lock while executing it.

Of course I could also do a `self in _active` check under a lock, but that is 
slightly more costly, than `self._started` check and not any more useful.

I may be prematurely optimizing things, but people tend to like they threads to 
startup ASAP.

--
Added file: http://bugs.python.org/file44677/issue27908_2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27908] del _limbo[self] KeyError

[Dima Tisnek]

Dima Tisnek added the comment:

Your logic is accurate; _started is in fact separate from _limbo.

As such taking a lock for "test-then-set" only would suffice.

Now when you bring the other primitive under this lock in one place, it would 
look cleaner if it was also brought in the other.



There's one more issue with proposed change:

Before the change, if "already started" exception is ever raised for a given 
Thread object, then it's guaranteed that that object was started successfully.

With the change, it is possible that exception is raised, and thread fails to 
start, leaving the object in initial state.


If it were up to me, I would buy this limitation as price of safety.
Someone may disagree.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27908] del _limbo[self] KeyError

[Brett Cannon]

Changes by Brett Cannon :


--
nosy:  -brett.cannon
versions: +Python 3.7

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27908] del _limbo[self] KeyError

[Brett Cannon]

Changes by Brett Cannon :


--
components:  -Extension Modules

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27908] del _limbo[self] KeyError

[Maciej Urbański]

Maciej Urbański added the comment:

@Dima.Tisnek, only reason for having both of these conditions together is so I 
won't have to repeat the same error message in the code at little price of the 
performance in this edge case (trying to start the same thread multiple times).

Unless I'm missing something there should be no way how it would make this lock 
required for setting `self._started`.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27908] del _limbo[self] KeyError

[Dima Tisnek]

Dima Tisnek added the comment:

@rooter, if you go this way, you should also `self._started.set()` with lock 
held, together with removing thread from `_limbo`

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27908] del _limbo[self] KeyError

[Maciej Urbański]

Maciej Urbański added the comment:

Successfully reproduced on 2.7.12 and 3.5.2 .
Currently there seems to be no protection against starting the same thread 
twice at the same time. What was checked was only if start operation already 
finished once.

Attached patch makes it so limbo, our starting threads' waiting room, is 
checked first.

--
keywords: +patch
nosy: +rooter
versions: +Python 2.7, Python 3.5
Added file: http://bugs.python.org/file44643/issue27908.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27908] del _limbo[self] KeyError

[Antoine Pitrou]

Changes by Antoine Pitrou :


--
nosy: +tim.peters

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27908] del _limbo[self] KeyError

[SilentGhost]

Changes by SilentGhost :


--
components: +Extension Modules
nosy: +brett.cannon
type:  -> behavior

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue27908] del _limbo[self] KeyError

[Dima Tisnek]

New submission from Dima Tisnek:

To reproduce:

```
import threading
import time


class U(threading.Thread):
def run(self):
time.sleep(1)
if not x.ident:
x.start()


x = U()
for u in [U() for i in range(1)]: u.start()

time.sleep(10)
```

Chance to reproduce ~20% in my setup.
This script has a data race (check then act on x.ident).
I expected it to occasionally hit `RuntimeError: threads can only be started 
once`

Instead, I get:
```
Unhandled exception in thread started by >
Traceback (most recent call last):
  File "/usr/lib64/python3.5/threading.py", line 882, in _bootstrap
self._bootstrap_inner()
  File "/usr/lib64/python3.5/threading.py", line 906, in _bootstrap_inner
del _limbo[self]
KeyError: 
```

--
components: Library (Lib)
messages: 274005
nosy: Dima.Tisnek
priority: normal
severity: normal
status: open
title: del _limbo[self] KeyError
versions: Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com