Skip to content

raise BrokenResourceError if lock owner has exited. #3080

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions src/trio/_sync.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from __future__ import annotations

import inspect
import math
from typing import TYPE_CHECKING, Protocol

Expand Down Expand Up @@ -571,11 +572,19 @@ def acquire_nowait(self) -> None:
"""

task = trio.lowlevel.current_task()

if self._owner is task:
raise RuntimeError("attempt to re-acquire an already held Lock")
elif self._owner is None and not self._lot:
# No-one owns it
self._owner = task
elif (
self._owner is not None
and inspect.getcoroutinestate(self._owner.coro) is inspect.CORO_CLOSED
):
raise trio.BrokenResourceError(
"Attempted acquire on Lock which will never be released, owner has exited.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Attempted acquire on Lock which will never be released, owner has exited.",
"Attempted acquire on Lock which will never be released as the owner has exited.",

I'm not sure this is the ideal message either way but the alternative I can think of isn't great either: "Deadlock detected: acquiring lock which will never be released."

)
else:
raise trio.WouldBlock

Expand Down
29 changes: 29 additions & 0 deletions src/trio/_tests/test_sync.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import pytest

from .. import _core
from .._core._tests.tutil import slow
from .._sync import *
from .._timeouts import sleep_forever
from ..testing import assert_checkpoints, wait_all_tasks_blocked
Expand Down Expand Up @@ -586,3 +587,31 @@ async def lock_taker() -> None:
await wait_all_tasks_blocked()
assert record == ["started"]
lock_like.release()


async def test_lock_acquire_unowned_lock() -> None:
"""Test that trying to acquire a lock whose owner has exited raises an error.
Partial fix for https://github.com/python-trio/trio/issues/3035
"""
lock = trio.Lock()
async with trio.open_nursery() as nursery:
nursery.start_soon(lock.acquire)
with pytest.raises(
trio.BrokenResourceError,
match="^Attempted acquire on Lock which will never be released, owner has exited.$",
):
await lock.acquire()


@slow
async def test_lock_multiple_acquire() -> None:
"""This currently hangs without throwing any error
See https://github.com/python-trio/trio/issues/3035
"""
lock = trio.Lock()
with pytest.raises(trio.TooSlowError):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about xfailing this test?

with trio.fail_after(1):
async with trio.open_nursery() as nursery:
nursery.start_soon(lock.acquire)
# this should probably raise trio.BrokenResourceError
nursery.start_soon(lock.acquire)
Loading