Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RLock undocumented behavior in case of multiple acquire #70795

Closed
smbrd mannequin opened this issue Mar 22, 2016 · 7 comments
Closed

RLock undocumented behavior in case of multiple acquire #70795

smbrd mannequin opened this issue Mar 22, 2016 · 7 comments
Labels
3.11 only security fixes docs Documentation in the Doc dir easy stdlib Python modules in the Lib dir

Comments

@smbrd
Copy link
Mannequin

smbrd mannequin commented Mar 22, 2016

BPO 26608
Nosy @MojoVampire, @iritkatriel

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2016-03-22.06:56:38.096>
labels = ['easy', '3.11', 'library', 'docs']
title = 'RLock undocumented behavior in case of multiple acquire'
updated_at = <Date 2021-06-18.14:35:22.245>
user = 'https://bugs.python.org/smbrd'

bugs.python.org fields:

activity = <Date 2021-06-18.14:35:22.245>
actor = 'iritkatriel'
assignee = 'docs@python'
closed = False
closed_date = None
closer = None
components = ['Documentation', 'Library (Lib)']
creation = <Date 2016-03-22.06:56:38.096>
creator = 'smbrd'
dependencies = []
files = []
hgrepos = ['348']
issue_num = 26608
keywords = ['easy']
message_count = 4.0
messages = ['262165', '262231', '270216', '396068']
nosy_count = 5.0
nosy_names = ['docs@python', 'josh.r', 'ahxxm', 'smbrd', 'iritkatriel']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = None
url = 'https://bugs.python.org/issue26608'
versions = ['Python 3.11']

Linked PRs

@smbrd
Copy link
Mannequin Author

smbrd mannequin commented Mar 22, 2016

The number of acquisitions must be the same as the number of releases or else lock will not be released for other threads leading to deadlock. This is not mentioned in documentation.

First acquisition returns boolean and further acquisitions return 1. This is also not mentioned in documentation.

@smbrd smbrd mannequin assigned docspython Mar 22, 2016
@smbrd smbrd mannequin added docs Documentation in the Doc dir stdlib Python modules in the Lib dir labels Mar 22, 2016
@MojoVampire
Copy link
Mannequin

MojoVampire mannequin commented Mar 23, 2016

Per the docs ( https://docs.python.org/3/library/threading.html#rlock-objects ):

"To unlock the lock, a thread calls its release() method. acquire()/release() call pairs may be nested; only the final release() (the release() of the outermost pair) resets the lock to unlocked and allows another thread blocked in acquire() to proceed."

The docs aren't super clear on the return type, but they aren't so overly specified as to make returning either True or 1 incorrect; they use lowercase "true" to describe the return value, which doesn't *have* to mean True, just something that evaluates as truthy.

In 3.5 at least, it looks like both initial and subsequent acquires are all returning True, even when called without passing an argument; this actually violates the docs, which claim that not passing an argument means "There is no return value" (possibly only when there is contended acquisition, the wording is odd), when in fact a no-argument call returns True just like explicitly passing blocking as True.

@ahxxm
Copy link
Mannequin

ahxxm mannequin commented Jul 12, 2016

As seen from commit log, all return type are double back-quoted, this could be a rendering error.

I think this commit somehow makes it clear that RLock is a thread-level reentrant lock, some code example of suggested usage might be helpful though.

@iritkatriel
Copy link
Member

The RLock documentation is a bit more verbose than it needs to be (for instance, there is no reason to specify the "no args" case separately from the "blocking=True" case (since True is the default value of blocking).

The issue of balancing acquire/release calls is mentioned as Josh says, but could perhaps be stated more simply as well.

@iritkatriel iritkatriel added easy 3.11 only security fixes labels Jun 18, 2021
@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
@samatjain
Copy link
Contributor

Working on this at PyCon 2023's cpython sprint.

@samatjain
Copy link
Contributor

samatjain commented Apr 25, 2023

Notes while trying to update the documentation:

First acquisition returns boolean and further acquisitions return 1. This is also not mentioned in documentation.

This longer seems to be true; acquire appears to always return True or False. Also, the documentation has a statement:

If more than one thread is blocked waiting until the lock is unlocked, only one at a time will be able to grab ownership of the lock. There is no return value in this case.

I have no idea what this means; why would it not return True or False? Confirmed w/ @gpshead it doesn't make sense. It's able to acquire the lock or not!

Here's a test program I have been fiddling with on Python 3.12:

import operator
import time
import threading

LEVEL_LIMIT: int = 10
THREADS_MAX = 16

global_rlock = threading.RLock()


def recursively_lock(level: int):
    """Test for RLock. Recusively call itself and observe the return value of acquire/release."""

    tid = threading.get_ident()

    if level == LEVEL_LIMIT:
        print(f"{LEVEL_LIMIT=} reached, stopping recursion")
        return

    print(f"Started {tid=}")

    rv = global_rlock.acquire(blocking=False)
    if rv:
        print(f"RLock acquire()={rv} at {level=} for {tid=}")
        time.sleep(0.1)

        recursively_lock(level + 1)

        rv = global_rlock.release()
        print(f"RLock release()={rv} at {level=}")
    else:
        print(f"Unable to acquire lock for {tid=} {rv=}")


if __name__ == "__main__":
    threads: list[threading.Thread] = []
    for i in range(0, THREADS_MAX):
        threads.append(threading.Thread(target=recursively_lock, args=(0,)))

    for t in threads:
        t.start()
    # map(operator.methodcaller("start"), threads)

    map(operator.methodcaller("join"), threads)
    print("Hello world")

I've been unable to replicate the case where None is returned, or acquire not returning True or False.

samatjain added a commit to samatjain/cpython that referenced this issue Apr 25, 2023
Attempted to simultaneously reduce verbosity, while more descriptively
describing behavior.

Fix links (RLock acquire/release previously linking to Lock
acquire/release, seems like bad copy pasta).
@samatjain
Copy link
Contributor

The old docs mention that acquire returns None, this seems not the case looking at rlock_acquire's implementation https://github.com/python/cpython/blob/main/Modules/_threadmodule.c#L355

Double checked by gpshead

AlexWaygood added a commit to samatjain/cpython that referenced this issue Apr 25, 2023
CAM-Gerlach added a commit to samatjain/cpython that referenced this issue May 22, 2024
CAM-Gerlach added a commit that referenced this issue May 22, 2024
Attempted to simultaneously reduce verbosity, while more descriptively
describing behavior.

Fix links (RLock acquire/release previously linking to Lock
acquire/release, seems like bad copy pasta).

Add a seealso for with-locks.

Switch section to use bullet points.

---------

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 22, 2024
Attempted to simultaneously reduce verbosity, while more descriptively
describing behavior.

Fix links (RLock acquire/release previously linking to Lock
acquire/release, seems like bad copy pasta).

Add a seealso for with-locks.

Switch section to use bullet points.

---------

(cherry picked from commit 2fbea81)

Co-authored-by: uıɐɾ ʞ ʇɐɯɐs <_@skj.io>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
miss-islington pushed a commit to miss-islington/cpython that referenced this issue May 22, 2024
Attempted to simultaneously reduce verbosity, while more descriptively
describing behavior.

Fix links (RLock acquire/release previously linking to Lock
acquire/release, seems like bad copy pasta).

Add a seealso for with-locks.

Switch section to use bullet points.

---------

(cherry picked from commit 2fbea81)

Co-authored-by: uıɐɾ ʞ ʇɐɯɐs <_@skj.io>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
CAM-Gerlach added a commit that referenced this issue May 22, 2024
gh-70795: Rework RLock documentation (GH-103853)

Attempted to simultaneously reduce verbosity, while more descriptively
describing behavior.

Fix links (RLock acquire/release previously linking to Lock
acquire/release, seems like bad copy pasta).

Add a seealso for with-locks.

Switch section to use bullet points.

---------

(cherry picked from commit 2fbea81)

Co-authored-by: uıɐɾ ʞ ʇɐɯɐs <_@skj.io>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
gpshead pushed a commit that referenced this issue May 22, 2024
gh-70795: Rework RLock documentation (GH-103853)

Attempted to simultaneously reduce verbosity, while more descriptively
describing behavior.

Fix links (RLock acquire/release previously linking to Lock
acquire/release, seems like bad copy pasta).

Add a seealso for with-locks.

Switch section to use bullet points.

---------

(cherry picked from commit 2fbea81)

Co-authored-by: uıɐɾ ʞ ʇɐɯɐs <_@skj.io>
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@hugovk hugovk closed this as completed Jun 15, 2024
estyxx pushed a commit to estyxx/cpython that referenced this issue Jul 17, 2024
Attempted to simultaneously reduce verbosity, while more descriptively
describing behavior.

Fix links (RLock acquire/release previously linking to Lock
acquire/release, seems like bad copy pasta).

Add a seealso for with-locks.

Switch section to use bullet points.

---------

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.11 only security fixes docs Documentation in the Doc dir easy stdlib Python modules in the Lib dir
Projects
None yet
Development

No branches or pull requests

3 participants