X Tutup
Skip to content

FIX: Resolve keypresslock race condition between multiple TextBox widgets with toolmanager#31268

Closed
herley-shaori wants to merge 2 commits intomatplotlib:mainfrom
herley-shaori:fix/textbox-keypresslock-race-condition
Closed

FIX: Resolve keypresslock race condition between multiple TextBox widgets with toolmanager#31268
herley-shaori wants to merge 2 commits intomatplotlib:mainfrom
herley-shaori:fix/textbox-keypresslock-race-condition

Conversation

@herley-shaori
Copy link

Summary

Fixes #29687.

When multiple TextBox widgets are used with toolmanager active (reproduced on QtAgg and TkAgg), rapidly clicking from one TextBox to another intermittently raises:

ValueError: already locked

Root Cause Analysis

button_press_event handlers are dispatched to all connected widgets in widget-creation order, not in click order. Consider two TextBox widgets, TB1 (created first) and TB2 (created second, currently active/typing):

  1. User clicks on TB1 while TB2 holds the keypresslock.
  2. TB1._click fires first (creation order) → calls begin_typing() → attempts to acquire keypresslockcrash, because TB2 still holds it.
  3. TB2._click fires second → would have called stop_typing() and released the lock, but it is already too late.

The instrumented trace from the issue reporter confirms this ordering:

TB2 BEGIN TYPING   ← (previous session, TB2 acquired focus)
TB1 BEGIN TYPING   ← TB1's _click fires first → ValueError: already locked
TB2 STOP TYPING    ← TB2's _click fires too late

Fix

Two targeted changes to lib/matplotlib/widgets.py:

1. LockDraw.owner property (new)

@property
def owner(self):
    """Return the current owner of the lock, or None if unlocked."""
    return self._owner

Exposes the current lock holder through a clean public API, avoiding the need for callers to reach into private _owner state.

2. TextBox.begin_typing() — graceful lock transfer

lock = toolmanager.keypresslock
if lock.locked() and not lock.isowner(self):
    if hasattr(lock.owner, 'stop_typing'):
        lock.owner.stop_typing()
toolmanager.keypresslock(self)
stack.callback(toolmanager.keypresslock.release, self)

Before acquiring the keypresslock, begin_typing() now checks whether another widget currently holds the lock. If so, it calls stop_typing() on that widget directly — releasing the lock and resetting the previous owner's typing state cleanly. When the previous owner's _click handler eventually fires stop_typing(), it becomes a no-op because capturekeystrokes is already False.

This implements the "force-grab focus" mechanism suggested by @anntzer in the issue discussion.

Test

Added test_TextBox_keypresslock_race_condition in test_widgets.py that directly reproduces the race scenario:

  • TB2 acquires the lock via begin_typing().
  • TB1 calls begin_typing() without TB2 having called stop_typing() first (simulating the event-order race).
  • Asserts that the lock transfers cleanly to TB1.
  • Asserts that TB2's subsequent stop_typing() is a no-op (does not raise, does not affect TB1's lock).
  • Asserts full cleanup after TB1.stop_typing().

Checklist

  • Regression test added
  • All existing TextBox tests pass (test_TextBox[none], test_TextBox[toolbar2], test_TextBox[toolmanager])
  • No changes to public API except the additive LockDraw.owner property

…gets with toolmanager (matplotlib#29687)

Problem
-------
When multiple TextBox widgets are used with toolmanager active (QtAgg, TkAgg),
rapidly clicking from one TextBox to another intermittently raises:

    ValueError: already locked

Root cause: button_press_event handlers are dispatched in widget-creation
order, not in click order.  When TextBox TB2 currently holds the keypresslock
(it is the active/typing widget) and the user clicks on TB1, TB1's _click
handler fires *before* TB2's _click handler.  TB1 calls begin_typing() and
tries to acquire the keypresslock while TB2 still holds it, causing the crash.
TB2's _click handler — which would have called stop_typing() and released the
lock — fires too late.

Fix
---
Two changes to lib/matplotlib/widgets.py:

1. LockDraw.owner property (new)
   Added a public `owner` property to `LockDraw` that exposes the current lock
   holder.  This gives callers a safe, API-level way to identify who holds the
   lock without reaching into private state.

2. TextBox.begin_typing() — graceful lock transfer
   Before acquiring the keypresslock, begin_typing() now checks whether the
   lock is already held by a *different* widget.  If so, it explicitly calls
   stop_typing() on that widget, which releases the lock and resets the
   previous owner's typing state in a clean, idempotent way.  When the
   previous owner's _click handler later fires stop_typing(), it is a no-op
   because capturekeystrokes is already False.

Test
----
Added test_TextBox_keypresslock_race_condition in test_widgets.py that directly
reproduces the race scenario: TB2 acquires the lock via begin_typing(), then
TB1 calls begin_typing() without TB2 having called stop_typing() first.
Asserts that the lock is transferred cleanly to TB1 and that TB2's subsequent
stop_typing() call is a no-op.
@github-actions
Copy link

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide.
Please let us know if (and how) you use AI, it will help us give you better feedback on your PR.

We strive to be a welcoming and open project. Please follow our Code of Conduct.

The `owner` property added to `LockDraw` in the previous commit was missing
from the type stub `lib/matplotlib/widgets.pyi`, causing mypy-stubtest to
report:

    error: matplotlib.widgets.LockDraw.owner is not present in stub

Added the corresponding property declaration to the stub so the runtime
implementation and the stub are in sync.
@anntzer
Copy link
Contributor

anntzer commented Mar 10, 2026

LLM output will not be reviewed. Repeated offenses will lead to banning.

@anntzer anntzer closed this Mar 10, 2026
@herley-shaori herley-shaori deleted the fix/textbox-keypresslock-race-condition branch March 10, 2026 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Development

Successfully merging this pull request may close these issues.

[Bug]: possible race condition between TextBoxes with toolmanager and QtAgg

2 participants

X Tutup