classification
Title: Improve semaphore documentation
Type: enhancement Stage: resolved
Components: Documentation, Library (Lib) Versions: Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: docs@python Nosy List: Garrett Berg, asvetlov, berker.peksag, docs@python, pitrou
Priority: normal Keywords: patch

Created on 2017-12-04 03:18 by Garrett Berg, last changed 2017-12-09 07:13 by berker.peksag. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 4709 merged Garrett Berg, 2017-12-05 00:38
PR 4750 merged python-dev, 2017-12-07 18:04
Messages (8)
msg307536 - (view) Author: Garrett Berg (Garrett Berg) * Date: 2017-12-04 03:17
The definition of threading.Semaphore is confusing (for all versions of python docs)

https://docs.python.org/2/library/threading.html#semaphore-objects

 acquire([blocking])

It currently states the following:

> When invoked without arguments: if the internal counter is larger than zero on entry, decrement it by one and return immediately. If it is zero on entry, block, waiting until some other thread has called release() to make it larger than zero. This is done with proper interlocking so that if multiple acquire() calls are blocked, release() will wake exactly one of them up. The implementation may pick one at random, so the order in which blocked threads are awakened should not be relied on. There is no return value in this case.

However, after testing it myself I found that is missing a crutial detail. Let's step through the docs:

> If the internal counter is larger than zero on entry, decrement it by one and return immediately.

This is exactly accurate and should stay the same

> If it is zero on entry, block, waiting until some other thread has called release() to make it larger than zero. This is done with proper interlocking so that if multiple acquire() calls are blocked, release() will wake exactly one of them up. The implementation may pick one at random, so the order in which blocked threads are awakened should not be relied on. There is no return value in this case.

This is extremely confusing and I would like to rewrite it as follows:

> If it is zero on entry block until awoken by a call to ``release()``. Once awoken, decrement the counter by 1. Exactly one thread will be awoken by a call to ``release()``. The order in which threads are awoken should not be relied on. ``None`` is returned in this case.

The major thing that was missing was that the **counter is decremented after the thread is awoken**. For instance, this code *generally* passes assertions:

```
#!/usr/bin/python2
import time
from threading import Thread, Semaphore

s = Semaphore(1)

def doit():
    s.acquire()
    print("did it")

th1 = Thread(target=doit)
th1.start()

th2 = Thread(target=doit)
th2.start()

time.sleep(0.2)

assert not th1.is_alive()
assert th2.is_alive()

s.release()
assert s._value == 1, "The value is increased to 1 MOMENTARILY"
start = time.time()
while sum([th2.is_alive(), th3.is_alive()]) > 1:
    assert time.time() - start < 0.5
    time.sleep(0.1)

assert s._value == 0, "when an aquire is awoken, THEN _value is decremented"
```

Obviously this behavior should not be relied on, but it gives a picture of what seems to be happening under the hood.

I came across this while trying to work through "The Little Book of Semaphores". After reading these docs I mistakingly thought that they didn't match Djestra's original semaphore since the values could not be negative. I now realize that while they may not match that implementation under the hood, they match it perfectly in practice since if you (for instance) ``acquire()`` 5 times and then ``release()`` 5 times the value of Semaphore._value will be the same when all is said and done.
msg307537 - (view) Author: Garrett Berg (Garrett Berg) * Date: 2017-12-04 03:20
Also, is "The order in which threads are awoken should not be relied on" actually true? Would it not be up to the OS specific scheduler? For instance, wouldn't the POSIX scheduler use a priority queue of some kind?
msg307538 - (view) Author: Garrett Berg (Garrett Berg) * Date: 2017-12-04 03:23
It may be desirable to change:

> Once awoken, decrement the counter by 1.

to

> Once awoke, decrement the counter (which is guaranteed to be positive) by 1.

Although this could be considered obvious since the counter can never go below zero.
msg307589 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-12-04 19:37
The changes you are proposing sound reasonable to me.  Would you like to submit a pull request for them?
msg307608 - (view) Author: Garrett Berg (Garrett Berg) * Date: 2017-12-05 00:38
Gave it a shot!
msg307819 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2017-12-07 18:04
New changeset a0374dd34aa25f0895195d388b5ceff43b121b00 by Andrew Svetlov (Garrett Berg) in branch 'master':
bpo-32208: update threading.Semaphore docs and add unit test (#4709)
https://github.com/python/cpython/commit/a0374dd34aa25f0895195d388b5ceff43b121b00
msg307820 - (view) Author: Andrew Svetlov (asvetlov) * (Python committer) Date: 2017-12-07 18:48
New changeset a04ca12e12b522850e7e9244c250754d3cd36f0a by Andrew Svetlov (Miss Islington (bot)) in branch '3.6':
bpo-32208: update threading.Semaphore docs and add unit test (GH-4709) (#4750)
https://github.com/python/cpython/commit/a04ca12e12b522850e7e9244c250754d3cd36f0a
msg307881 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2017-12-09 07:13
According to the discussion at https://github.com/python/cpython/pull/4709#issuecomment-350055732 it was decided to not backport this to 2.7. Closing this as 'fixed'. Thank you, all!
History
Date User Action Args
2017-12-09 07:13:48berker.peksagsetstatus: open -> closed

versions: - Python 2.7
nosy: + berker.peksag

messages: + msg307881
resolution: fixed
stage: patch review -> resolved
2017-12-07 18:48:37asvetlovsetmessages: + msg307820
2017-12-07 18:04:35python-devsetpull_requests: + pull_request4653
2017-12-07 18:04:30asvetlovsetnosy: + asvetlov
messages: + msg307819
2017-12-05 00:38:49Garrett Bergsetkeywords: + patch

stage: needs patch -> patch review
messages: + msg307608
pull_requests: + pull_request4620
2017-12-04 19:37:22pitrousetassignee: docs@python
components: + Documentation
versions: - Python 3.4, Python 3.5, Python 3.8
nosy: + docs@python, pitrou

messages: + msg307589
stage: needs patch
2017-12-04 06:07:52serhiy.storchakasettitle: Improve semaphore docmentation -> Improve semaphore documentation
2017-12-04 03:23:24Garrett Bergsetmessages: + msg307538
2017-12-04 03:20:46Garrett Bergsetmessages: + msg307537
2017-12-04 03:18:00Garrett Bergcreate