raise BrokenResourceError if lock owner has exited.

[jakkdl]

Partially addresses #3035

• Idk if adding test_lock_multiple_acquire is very meaningful
• need to add newsfragment

It's possible that a full implementation for #3035 makes this redundant, but this was easy enough to write&test I thought I might as well push it as a PR that improves the status quo for now.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.60%. Comparing base (51b2dff) to head (41a298a).
Report is 68 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3080   +/-   ##
=======================================
  Coverage   99.60%   99.60%           
=======================================
  Files         121      121           
  Lines       17882    17900   +18     
  Branches     3214     3221    +7     
=======================================
+ Hits        17811    17829   +18     
  Misses         50       50           
  Partials       21       21           

Files with missing lines	Coverage Δ
src/trio/_sync.py	100.00% <100.00%> (ø)
src/trio/_tests/test_sync.py	100.00% <100.00%> (ø)

[A5rocks]

Can you add a newsfragment?

[A5rocks]

How about xfailing this test?

[A5rocks]

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."

[Zac-HD]

I'm somewhat concerned about the performance impact of inspect.getcoroutinestate in acquire_nowait(), which might be fine.

The larger problem is that it only addresses the case where the holding task is already gone at acquire-time; I think we should probably go directly to the general solution of adding a ParkingLot.mark_resource_broken() method and using that from Lock.

[A5rocks]

I'm somewhat concerned about the performance impact of inspect.getcoroutinestate in acquire_nowait(), which might be fine.

Good point, if someone's polling acquire_nowait(). I think inspect.getcoroutinestate is decently fast but yeah having a way to mark a ParkingLot as poisoned/broken would be faster. ... Though if someone's polling acquire_nowait and that doesn't check the owner status, that's weird so maybe not.

The larger problem is that it only addresses the case where the holding task is already gone at acquire-time; I think we should probably go directly to the general solution of adding a ParkingLot.mark_resource_broken() method and using that from Lock.

Yeah, the problem is we can't just add a way to poison a ParkingLot, we also need a way to know when Lock's task gets ended. We can't use a weakref.finalizer because no guarantees on when Task gets deleted (it could be stored somewhere else other than _owner), so I think we need something in trio's event loop itself.

[jakkdl]

I'm somewhat concerned about the performance impact of inspect.getcoroutinestate in acquire_nowait(), which might be fine.

I ran a simple benchmark now and could not detect any difference with or without a call to inspect.getcoroutinestate.

The larger problem is that it only addresses the case where the holding task is already gone at acquire-time; I think we should probably go directly to the general solution of adding a ParkingLot.mark_resource_broken() method and using that from Lock.

Yes this PR intentionally only made the easy fix from #3035 (comment), but if the full fix turns out doable then we'll close this one.

[jakkdl]

This will probably end up closed in favor of #3081

[A5rocks]

Now unnecessary.