msg236215 - (view) |
Author: Teodor Dima (td) |
Date: 2015-02-19 11:31 |
The keyword for 'blocking' in the documentation for multiprocessing.Lock.acquire() (and all synchronization objects dependent on SemLock) differs from its implementation at Modules/_multiprocessing/semaphore.c:70 - https://docs.python.org/3.4/library/threading.html#threading.Lock.acquire
|
msg236332 - (view) |
Author: Davin Potts (davin) * |
Date: 2015-02-20 21:27 |
Interesting! The documentation in 3.4 as well as 2.7 indicates that the keyword should be 'blocking' yet the code implements this as 'block'.
Code to reproduce empirically what is actually implemented:
import multiprocessing
dummy_lock = multiprocessing.Lock()
dummy_lock.acquire(blocking=False) # Raises a TypeError on invalid keyword
The same code changed to 'block=False' works happily.
The code should be changed to reflect the docs and a test probably added too that both exercises this keyword explicitly by name and tests to see if we've fallen out of sync with the threading module.
|
msg236348 - (view) |
Author: Davin Potts (davin) * |
Date: 2015-02-21 03:26 |
Of course, there's code in the wild that expects and uses the parameter named 'block' so simply changing this keyword will result in breaking others' code.
Two potentially appealing options:
1) Document that acquire in multiprocessing differs from threading in this way.
2) Implement 'blocking' as a supported keyword argument though preserve support for 'block' as a deprecated keyword.
|
msg236431 - (view) |
Author: Teodor Dima (td) |
Date: 2015-02-23 08:47 |
>>> Of course, there's code in the wild that expects and uses the parameter named 'block' so simply changing this keyword will result in breaking others' code.
That is, indeed, the case with my company's code as well.
|
msg236986 - (view) |
Author: Davin Potts (davin) * |
Date: 2015-03-01 21:53 |
In the docs for 2.7, multiprocessing.Lock refers to threading.Lock in which the signature for acquire looks like:
threading.Lock.acquire([blocking])
However, in the current code in 2.7, Modules/_multiprocessing/semaphore.c implements multiprocessing.Lock.acquire to support two arguments: 'block' and 'timeout'.
In the docs for 3.4, threading.Lock.acquire now has both 'blocking' and 'timeout' arguments. Notably, it specifies: "It is forbidden to specify a timeout when blocking is false." This is indeed enforced by threading.Lock but not at all by multiprocessing.Lock. In a 3.4.3 shell:
>>> import multiprocessing
>>> import threading
>>> ml = multiprocessing.Lock()
>>> ml.acquire(False, 20.0)
True
>>> ml.release()
>>> tl = threading.Lock()
>>> tl.acquire(False, 20.0)
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
ValueError: can't specify a timeout for a non-blocking call
In 2.7, acquire similarly permits a supplied value for timeout even when set to be non-blocking.
A further difference: the code in Modules/_multiprocessing/semaphore.c appears to translate a timeout=None into an infinite wait whereas the threading implementation rejects any attempt to supply a None for timeout, demanding a float instead. (In 3.4, threading uses timeout=-1 to request an infinite wait.)
Given these discrepancies between threading's and multiprocessing's implementations for Lock and given the difficulties in renaming an argument that can be supplied as a non-keyword parameter, the right thing to do at this point is to properly document multiprocessing.Lock's acquire as using 'block' and 'timeout'.
|
msg236991 - (view) |
Author: Davin Potts (davin) * |
Date: 2015-03-01 22:20 |
To be fair, the docs in 2.7 do actually mention the use of 'block' instead of 'blocking' in acquire though it does so inside a "Note" block a bit later in the docs after first claiming that multiprocessing.Lock is a "clone".
In 3.4, that important detail has been inexplicably stripped from this distended "Note" block.
Ultimately, this use of the "Note" block seems to misplace important information that should appear in the description of the multiprocessing.Lock class and its siblings in the synchronization section.
|
msg239339 - (view) |
Author: Davin Potts (davin) * |
Date: 2015-03-26 15:32 |
Attaching patches for 3.5/default, 3.4, and 2.7 which update the documentation on multiprocessing.Lock, RLock, Semaphore, and BoundedSemaphore to describe their actual implemented behavior, replacing the existing, misleading claim that they are clones of threading.Lock, etc., respectively.
Specific changes:
* Lock and RLock now have their acquire and release methods drawn out so as to best explain the significant behavioral differences (versus threading) in return values, potential exceptions being raised, and passing arguments.
* Semaphore and BoundedSemaphore are not as drawn out because their behaviors are indeed the same in every regard except for the difference in block-vs-blocking (and in the case of 2.7, timeout is novel in multiprocessing and doesn't appear until 3.2 in threading).
* In 3.4 and 3.5, the inaccurate note block at the end of the synchronization section no longer misrepresents the treatment of the timeout argument in threading analogs.
It is worth noting that in 3.x, the behavior around the timeout argument in multiprocessing.{Lock,RLock,Semaphore,BoundedSemaphore} already matches that of threading.Semaphore and BoundedSemaphore. The threading module appears inconsistent in its use of the timeout argument when comparing Semaphore/BoundedSemaphore with Lock/RLock. It would be interesting to investigate whether in 3.x this inconsistency within threading could be brought in line with the rest of multiprocessing.
In issue21342 the notion of unifying the implementations of these synchronization primitives across threading and multiprocessing is suggested. If that is pursued, it would reasonably be attempted on default/3.5 and not backported to 2.7 (nor probably 3.4). Thus this patch to update the documentation is still highly valuable for 2.7 and 3.4 and 3.5 (at least until such time as a unification effort might be undertaken in the case of 3.5).
|
msg239770 - (view) |
Author: Davin Potts (davin) * |
Date: 2015-04-01 04:34 |
@berker: I would have said this should not be marked an enhancement as the proposed solution (in the patch) is to correct the errors in the documentation to accurately describe the current implemented behavior. Does that make sense?
Whatever label we give it, do you think my logic (described in my comments on this issue) for why it is best to fix it this way makes sense to you?
|
msg239809 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2015-04-01 13:46 |
behavior vs enhancment for doc changes is really pretty meaningless/arbitrary, IMO.
|
msg239821 - (view) |
Author: Berker Peksag (berker.peksag) * |
Date: 2015-04-01 15:31 |
I use "enhancement" for non-trivial documentation patches.
Also, quoting from https://docs.python.org/devguide/triaging.html#type
"Also used for improvements in the documentation and test suite and for other refactorings."
In this case, your patch fixes a documentation issue and improving current documentation by documenting {Lock,RLock}.acquire() and release() methods properly (documentation improvements can go into bugfix branches).
|
msg239929 - (view) |
Author: Davin Potts (davin) * |
Date: 2015-04-02 15:49 |
@berker: Sadly, I've read those descriptions in triaging.html more than once and that part apparently did not stick in my head. Hopefully it will now -- thanks.
@r.david: Ok, cool -- I had been mentally associating more significance to one versus the other but that helps to understand.
As Berker correctly suspected, I would not have expected something labelled "Enhancement" to be allowed into the 2.7 branch so the explanation definitely helps.
|
msg240283 - (view) |
Author: Davin Potts (davin) * |
Date: 2015-04-08 20:27 |
Updating patch for default/3.5 and 3.4 branches to remove versionchanged message on {Lock,RLock}.acquire per feedback from @berker.
|
msg240284 - (view) |
Author: Davin Potts (davin) * |
Date: 2015-04-08 20:28 |
Same for 2.7 branch.
|
msg250449 - (view) |
Author: Davin Potts (davin) * |
Date: 2015-09-11 04:53 |
@berker.peksag, @r.david.murray: Could I gently nudge one of you into committing the final versions of these patches?
|
msg251195 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2015-09-21 03:52 |
New changeset f3faf0f355e0 by Berker Peksag in branch '3.4':
Issue #23484: Document differences between synchronization primitives of
https://hg.python.org/cpython/rev/f3faf0f355e0
New changeset 6cd030099966 by Berker Peksag in branch '3.5':
Issue #23484: Document differences between synchronization primitives of
https://hg.python.org/cpython/rev/6cd030099966
New changeset 020377a15708 by Berker Peksag in branch 'default':
Issue #23484: Document differences between synchronization primitives of
https://hg.python.org/cpython/rev/020377a15708
|
msg251197 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2015-09-21 04:15 |
New changeset 92350a2a8b08 by Berker Peksag in branch '2.7':
Issue #23484: Document differences between synchronization primitives of
https://hg.python.org/cpython/rev/92350a2a8b08
|
msg251198 - (view) |
Author: Berker Peksag (berker.peksag) * |
Date: 2015-09-21 04:17 |
Thanks, Teodor and Davin!
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:12 | admin | set | github: 67672 |
2021-06-21 12:03:02 | iritkatriel | link | issue21342 superseder |
2015-09-21 04:17:00 | berker.peksag | set | status: open -> closed resolution: fixed messages:
+ msg251198
stage: patch review -> resolved |
2015-09-21 04:15:49 | python-dev | set | messages:
+ msg251197 |
2015-09-21 03:52:10 | python-dev | set | nosy:
+ python-dev messages:
+ msg251195
|
2015-09-11 09:52:30 | berker.peksag | set | assignee: docs@python -> berker.peksag versions:
+ Python 3.6 |
2015-09-11 04:53:03 | davin | set | messages:
+ msg250449 |
2015-04-08 20:28:24 | davin | set | files:
+ issue_23484_doc_locks_py27_noverchange.patch
messages:
+ msg240284 |
2015-04-08 20:27:53 | davin | set | files:
+ issue_23484_doc_locks_py35_and_py34_noverchange.patch
messages:
+ msg240283 |
2015-04-02 15:49:49 | davin | set | messages:
+ msg239929 |
2015-04-01 15:31:05 | berker.peksag | set | messages:
+ msg239821 |
2015-04-01 13:46:40 | r.david.murray | set | nosy:
+ r.david.murray messages:
+ msg239809
|
2015-04-01 04:34:09 | davin | set | messages:
+ msg239770 |
2015-03-31 02:40:03 | berker.peksag | set | nosy:
+ berker.peksag type: behavior -> enhancement
|
2015-03-26 15:37:48 | davin | set | nosy:
+ rhettinger
|
2015-03-26 15:33:31 | davin | set | files:
+ issue_23484_doc_locks_py27.patch |
2015-03-26 15:32:40 | davin | set | files:
+ issue_23484_doc_locks_py35_and_py34.patch keywords:
+ patch messages:
+ msg239339
stage: needs patch -> patch review |
2015-03-02 21:03:46 | davin | link | issue21342 dependencies |
2015-03-01 22:20:20 | davin | set | messages:
+ msg236991 |
2015-03-01 21:53:05 | davin | set | nosy:
+ docs@python messages:
+ msg236986
assignee: docs@python components:
+ Documentation, - Extension Modules |
2015-02-23 08:47:41 | td | set | messages:
+ msg236431 |
2015-02-21 03:26:13 | davin | set | messages:
+ msg236348 |
2015-02-20 21:27:45 | davin | set | stage: needs patch messages:
+ msg236332 versions:
+ Python 2.7, Python 3.5 |
2015-02-20 09:39:24 | ned.deily | set | nosy:
+ sbt, davin
|
2015-02-19 11:31:14 | td | create | |