Add Event Watchers and Filters#3637
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an event-watcher API: C implementation and module bindings for adding/removing Python watcher callbacks, type stub entries, documentation, C documentation macros, deferred-error handling for watcher exceptions, and unit tests exercising watcher add/remove behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Application
participant Pygame as Pygame C Layer
participant SDL as SDL Event System
participant Watcher as User Watcher Callback
rect rgba(230,240,255,0.5)
App->>Pygame: add_event_watcher(watcher_fn)
Pygame->>Pygame: store watcher in _pg_event_watcher_list
Pygame->>SDL: register C-level event watch callback
end
rect rgba(220,255,220,0.5)
SDL->>Pygame: event generated (SDL_Event)
Pygame->>Pygame: pg_eventNew_no_free_proxy(SDL_Event)
Pygame->>Watcher: call watcher_fn(Event) (GIL acquired)
alt watcher raises exception
Pygame->>Pygame: capture to _pg_deferred_error (defer)
end
Pygame->>Pygame: CHECK_AND_RAISE_DEFERRED_ERROR at next poll/pump/etc.
end
rect rgba(255,235,220,0.5)
App->>Pygame: remove_event_watcher(watcher_fn)
Pygame->>Pygame: remove watcher from list
Pygame->>SDL: unregister C-level event watch callback (if needed)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src_c/event.c (1)
718-751: Consider optimizing Event object creation in filter loop.Line 731 creates a new Event object for every filter in every iteration. This could impact performance when multiple filters are registered and many events are processed.
Consider creating the Event object once before the loop and reusing it for all filter calls, unless there's a specific reason each filter needs a fresh Event object.
# Pseudo-code suggestion: PyObject *eventObj = pg_eventNew_no_free_proxy(event); for (Py_ssize_t i = 0; i < totalUserFilters; i++) { PyObject *filter = PyList_GetItem(userFilters, i); PyObject *returnValue = PyObject_CallOneArg(filter, eventObj); # ... error handling and skip logic } Py_DECREF(eventObj);However, verify this is safe - if filters can modify the Event object, creating a new one each time may be intentional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
buildconfig/stubs/pygame/event.pyi(2 hunks)docs/reST/ref/event.rst(1 hunks)src_c/doc/event_doc.h(1 hunks)src_c/event.c(6 hunks)test/event_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/event_test.py (2)
src_c/event.c (1)
event(2751-2822)buildconfig/stubs/pygame/event.pyi (8)
add_event_watcher(56-56)clear(47-47)post(54-54)Event(28-29)poll(44-44)remove_event_watcher(57-57)add_event_filter(58-58)remove_event_filter(59-59)
src_c/event.c (1)
src_c/include/pythoncapi_compat.h (1)
PyObject_CallOneArg(398-401)
🪛 Ruff (0.14.3)
test/event_test.py
942-942: Unused function argument: event
(ARG001)
958-958: Unused function argument: event
(ARG001)
976-976: Unused function argument: event
(ARG001)
🔇 Additional comments (11)
test/event_test.py (4)
938-970: LGTM!The test correctly verifies that event watchers are called when events are posted. The static analysis warning about unused
eventparameters is a false positive - the test intentionally doesn't use the event parameter, as it's only verifying that the watchers are invoked.
971-990: LGTM!The test correctly verifies that removing an event watcher prevents it from being called on subsequent events.
992-1011: LGTM!The test correctly verifies that event filters can selectively block events from reaching the queue.
1013-1039: LGTM!The test correctly verifies that removing an event filter restores normal event processing and that attempting to remove a non-existent filter raises
ValueError.src_c/event.c (7)
103-104: LGTM!The global
userFilterslist is properly declared and initialized at line 814 inpgEvent_AutoInit.
523-525: LGTM!The forward declaration is necessary since
pg_eventNew_no_free_proxyis used in the event filter before its definition.
2547-2583: LGTM - Complex but correct reference counting.The function correctly manages the
dict_proxyreference counting to prevent double-free issues when multiple parts of the codebase use the same event. The temporary manipulation ofdo_free_at_endis necessary to ensure the proxy isn't freed prematurely.
2585-2625: LGTM!The watcher wrapper correctly handles GIL acquisition, error handling, and SDL2 compatibility. Errors are appropriately printed to stderr and cleared to prevent them from propagating into SDL's event processing.
2627-2648: LGTM!The function correctly validates the callable and registers it with SDL. Returning the argument with INCREF enables decorator usage.
2665-2687: LGTM!The function correctly handles thread safety with GIL and mutex, and returns the argument with INCREF for decorator usage.
2738-2745: LGTM!The method table entries correctly expose the new event watcher and filter functions to Python.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
docs/reST/ref/event.rst (1)
471-490: Consolidate version markers and minor documentation issues.All four new function definitions use
versionaddedold:: todoas a placeholder. These need to be updated to:
- Replace "todo" with the actual version number this PR targets (e.g.,
3.0.0or the next release)- Consider whether
versionaddedoldis the intended directive; elsewhere in the file,versionaddedis used for new features (without the "old" suffix)Additionally, the typos on lines 481 and 515 ("seperate" → "separate") flagged in previous reviews have already been corrected in this revision.
Please verify:
- What version number should replace "todo"?
- Should the directive be
versionadded(to match modern sections) orversionaddedold(for consistency with legacy event docs)?Once confirmed, apply a consolidated diff for all four functions:
- .. versionaddedold:: todo + .. versionadded:: X.X.XAlso applies to: 492-503, 505-523, 525-535
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/reST/ref/event.rst(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-11T10:24:52.717Z
Learnt from: damusss
Repo: pygame-community/pygame-ce PR: 3604
File: buildconfig/stubs/pygame/surface.pyi:298-299
Timestamp: 2025-10-11T10:24:52.717Z
Learning: For the pygame-community/pygame-ce repository, batch similar documentation or terminology improvements together in a single comment rather than pointing them out individually. This helps avoid unnecessary commit spam.
Applied to files:
docs/reST/ref/event.rst
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src_c/event.c (2)
2650-2663: Critical: Missing Py_INCREF for returned Py_None.Line 2662 returns
Py_Nonewithout incrementing its reference count, which will cause reference counting errors and potential crashes.Apply this diff:
- return Py_None; + Py_RETURN_NONE;
2689-2707: Critical: Missing Py_INCREF for returned Py_None.Line 2706 returns
Py_Nonewithout incrementing its reference count, which will cause reference counting errors and potential crashes.Apply this diff:
- Py_RETURN_NONE; + Py_RETURN_NONE;Note: The
Py_RETURN_NONEmacro handles the reference count correctly.
🧹 Nitpick comments (2)
src_c/event.c (2)
2674-2678: Minor: Unnecessary GIL acquisition in Python-called function.
pg_event_add_filteris called from Python via the module method table, so the GIL is already held. ThePyGILState_Ensure()andPyGILState_Release()calls at lines 2674 and 2678 are unnecessary and add overhead.Apply this diff:
if (PyCallable_Check(arg)) { - PyGILState_STATE gstate = PyGILState_Ensure(); PG_LOCK_EVFILTER_MUTEX PyList_Append(userFilters, arg); PG_UNLOCK_EVFILTER_MUTEX - PyGILState_Release(gstate); }
2694-2705: Minor: Unnecessary GIL acquisition in Python-called function.Similar to
pg_event_add_filter,pg_event_remove_filteris called from Python, so the GIL is already held. ThePyGILState_Ensure()andPyGILState_Release()calls are unnecessary.Apply this diff:
VIDEO_INIT_CHECK(); - PyGILState_STATE gstate = PyGILState_Ensure(); PG_LOCK_EVFILTER_MUTEX Py_ssize_t index = PySequence_Index(userFilters, arg); if (index == -1) { PG_UNLOCK_EVFILTER_MUTEX PyErr_SetString(PyExc_ValueError, "event filter not found"); - PyGILState_Release(gstate); return NULL; } PySequence_DelItem(userFilters, index); PG_UNLOCK_EVFILTER_MUTEX - PyGILState_Release(gstate); Py_RETURN_NONE;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src_c/event.c(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-13T12:39:19.130Z
Learnt from: oddbookworm
Repo: pygame-community/pygame-ce PR: 3556
File: src_c/mixer.c:806-836
Timestamp: 2025-08-13T12:39:19.130Z
Learning: In pygame-ce's mixer.c, the snd_copy function requires Py_INCREF(newSound) before returning to maintain proper reference counting. Without it, copied Sound objects get deallocated prematurely.
Applied to files:
src_c/event.c
📚 Learning: 2025-09-01T20:18:57.500Z
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_pygame.h:108-133
Timestamp: 2025-09-01T20:18:57.500Z
Learning: In pygame-ce PR #3573, PG_SURF_BitsPerPixel was changed from a simple macro to a static inline function to handle SDL2/SDL3 differences. SDL2's BitsPerPixel includes padding bits (e.g., RGBX => 32) while SDL3's SDL_BITSPERPIXEL excludes padding (e.g., RGBX => 24). The new implementation bridges this gap rather than removing the helper entirely.
Applied to files:
src_c/event.c
🧬 Code graph analysis (1)
src_c/event.c (1)
src_c/include/pythoncapi_compat.h (1)
PyObject_CallOneArg(398-401)
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src_c/event.c(6 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-13T12:39:19.130Z
Learnt from: oddbookworm
Repo: pygame-community/pygame-ce PR: 3556
File: src_c/mixer.c:806-836
Timestamp: 2025-08-13T12:39:19.130Z
Learning: In pygame-ce's mixer.c, the snd_copy function requires Py_INCREF(newSound) before returning to maintain proper reference counting. Without it, copied Sound objects get deallocated prematurely.
Applied to files:
src_c/event.c
📚 Learning: 2025-09-01T20:18:57.500Z
Learnt from: Starbuck5
Repo: pygame-community/pygame-ce PR: 3573
File: src_c/_pygame.h:108-133
Timestamp: 2025-09-01T20:18:57.500Z
Learning: In pygame-ce PR #3573, PG_SURF_BitsPerPixel was changed from a simple macro to a static inline function to handle SDL2/SDL3 differences. SDL2's BitsPerPixel includes padding bits (e.g., RGBX => 32) while SDL3's SDL_BITSPERPIXEL excludes padding (e.g., RGBX => 24). The new implementation bridges this gap rather than removing the helper entirely.
Applied to files:
src_c/event.c
🧬 Code graph analysis (1)
src_c/event.c (1)
src_c/include/pythoncapi_compat.h (1)
PyObject_CallOneArg(398-401)
🔇 Additional comments (1)
src_c/event.c (1)
815-818: LGTM: Proper initialization and error handling.The initialization of
userFilterscorrectly checks for NULL and raises an appropriate error if allocation fails.
There was a problem hiding this comment.
Hello and welcome, thanks for taking the time to contribute to pygame-ce 🥳 It is really impressive to see a new contributor directly tacking deep C code 😎
Some general notes:
- Ideally it would have been better if you opened a new issue to discuss the API before implementing it.
- SDL3, which is what we are porting pygame-ce to, has the new main callbacks API. Ideally, any new API we add must follow SDL3 style/behaviour even if implemented in the current SDL2 codebase.
- For this PR, I am not opposed to adding event callbacks but I believe there should be only one thing added, and that should be equivalent to SDL3's
SDL_AppEvent. Adding both watchers and filters is exposing too much details to the users and is potentially more confusing IMHO.
This is just my idea but here's what a pygame-ce app using a main callback API could look like in the future (of course, this is not fully fleshed out either)
import pygame
@pygame.app_event
def handle_event(event: pygame.Event):
if event.type == pygame.QUIT:
raise SystemExit
... # handle any other event here
@pygame.app_iterate
def handle_loop(window: pygame.Window):
pygame.draw.xxx(window.get_surface(), ...)
... # do other things
# no need to call pygame.display.update/flip or window flip
pygame.launch([size], [title], [other set_mode params])We were planning to tackle this when we got to using SDL3 completely.
Fair enough, I'll try to keep that in mind for the future. Should I make a new issue to discuss this there?
I do remember reading that somewhere, would you suggest making the event watchers behave more like SDL_AppEvent?
Also pretty fair, I mostly just added the filters because I considered them an adjacent feature that I figured I would add while I was adding the event watchers. If you think that they are making things more confusing I have no problem with removing them. |
yes that is what I would personally prefer.
That would be appreciated, I would also like to hear other contributors opinions on this API. |
|
A registerable callback like Also see another large event API change proposal, #2759. |
aatle
added
New API
That's actually why I implemented it the way I did, it allows for the normal event loop to be used as-is, while offering an alternative completely optionally. My goal for this particular bit of API was to add the functionality without having any impact on anything else.
That's actually pretty cool, maybe the function's signature could support specifying event types to target? @pygame.add_event_watcher(KeydownEvent)
def keydownListener(event: KeydownEvent):
... # use the event somehow |
|
For the time being, I think that a simple general-purpose interface to the underlying SDL function is best. |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
test/event_test.py (1)
944-976: Clean up watchers after test to ensure isolation.The test adds watchers but never removes them. While
pygame.display.quit()intearDowneventually triggers cleanup, explicitly removing watchers at the end of the test would improve test isolation and make the cleanup explicit.Also, line 962's string literal (
"""Test multiple event watchers""") is not a docstring since it appears after executable code—it's just a discarded string. Consider converting it to a comment:self.assertEqual(counter, 1) - """Test multiple event watchers""" + # Test multiple event watchers🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/event_test.py` around lines 944 - 976, The test leaves event watchers registered and contains a stray string used like a comment; after adding watchers with pygame.event.add_event_watcher(eventWatcher) and pygame.event.add_event_watcher(otherEventWatcher) remove them at the end of the test by calling pygame.event.remove_event_watcher(eventWatcher) and pygame.event.remove_event_watcher(otherEventWatcher) to ensure isolation, and replace the discarded string literal """Test multiple event watchers""" with a real inline comment (e.g. # Test multiple event watchers).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@buildconfig/stubs/pygame/event.pyi`:
- Around line 59-60: The stub for remove_event_watcher currently declares a None
return but the C implementation pg_event_remove_watcher (src_c/event.c) returns
True/False; update the stub signature for remove_event_watcher(watcher: T, /) to
return bool (i.e., -> bool) so the type matches the C function's success/failure
boolean result; ensure the change mirrors the existing add_event_watcher
signature style and keep the generic T parameter unchanged.
In `@src_c/event.c`:
- Around line 2668-2674: The deferred-error path in src_c/event.c is leaking a
reference because PyErr_GetRaisedException() already returns a new reference
which is being incremented again by Py_INCREF(_pg_deferred_error); remove the
extra Py_INCREF call so _pg_deferred_error holds the single new reference
returned by PyErr_GetRaisedException(), and ensure later code that calls
PyErr_SetRaisedException consumes/steals exactly one reference as intended
(references involved: PyErr_GetRaisedException, _pg_deferred_error,
PyErr_SetRaisedException).
- Around line 2621-2628: Release the lock before calling pgEvent_New is fine,
but the write to dict_proxy->do_free_at_end must be protected: after creating
eventObj with pgEvent_New(event) re-acquire the same lock
(SDL_AtomicLock(&dict_proxy->lock)), restore dict_proxy->do_free_at_end from
proxy_frees_on_end, then unlock with SDL_AtomicUnlock(&dict_proxy->lock); this
ensures the write is synchronized while still allowing the lock to be released
during the potentially blocking pgEvent_New call; keep num_on_queue behavior
unchanged.
- Line 823: In pgEvent_AutoQuit, before the existing
Py_DECREF(_pg_event_watcher_list) call, iterate over each entry in
_pg_event_watcher_list and call SDL_RemoveEventWatch(pg_watcher_wrapper,
watcher) to unregister it (same pattern used by pg_event_remove_watcher) so SDL
won't call pg_watcher_wrapper after the Python objects are DECREF'd; then
proceed to Py_DECREF(_pg_event_watcher_list) and SDL_Quit as before.
In `@test/event_test.py`:
- Around line 977-996: The test_remove_event_watcher docstring is misplaced
after pygame.event.clear() so move the triple-quoted string immediately below
the def test_remove_event_watcher(self): line to make it a proper docstring;
also add explicit cleanup to guarantee watcher removal (call
pygame.event.remove_event_watcher(eventWatcher) in a finally block or after
assertions) so that eventWatcher is always unregistered even if the test
fails—refer to the eventWatcher function and the pygame.event.add_event_watcher
/ pygame.event.remove_event_watcher calls to locate where to add the cleanup.
---
Nitpick comments:
In `@test/event_test.py`:
- Around line 944-976: The test leaves event watchers registered and contains a
stray string used like a comment; after adding watchers with
pygame.event.add_event_watcher(eventWatcher) and
pygame.event.add_event_watcher(otherEventWatcher) remove them at the end of the
test by calling pygame.event.remove_event_watcher(eventWatcher) and
pygame.event.remove_event_watcher(otherEventWatcher) to ensure isolation, and
replace the discarded string literal """Test multiple event watchers""" with a
real inline comment (e.g. # Test multiple event watchers).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7bac42c8-80a5-4008-af18-e5d3eff14b55
⛔ Files ignored due to path filters (1)
src_c/doc/event_doc.his excluded by!src_c/doc/*
📒 Files selected for processing (4)
buildconfig/stubs/pygame/event.pyidocs/reST/ref/event.rstsrc_c/event.ctest/event_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/reST/ref/event.rst
| def add_event_watcher(watcher: T, /) -> T: ... | ||
| def remove_event_watcher(watcher: T, /) -> None: ... |
There was a problem hiding this comment.
Return type mismatch for remove_event_watcher.
The C implementation (pg_event_remove_watcher in src_c/event.c:2708-2738) returns True when removal succeeds and False when the watcher is not found. The stub should reflect this:
def add_event_watcher(watcher: T, /) -> T: ...
-def remove_event_watcher(watcher: T, /) -> None: ...
+def remove_event_watcher(watcher: T, /) -> bool: ...📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def add_event_watcher(watcher: T, /) -> T: ... | |
| def remove_event_watcher(watcher: T, /) -> None: ... | |
| def add_event_watcher(watcher: T, /) -> T: ... | |
| def remove_event_watcher(watcher: T, /) -> bool: ... |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@buildconfig/stubs/pygame/event.pyi` around lines 59 - 60, The stub for
remove_event_watcher currently declares a None return but the C implementation
pg_event_remove_watcher (src_c/event.c) returns True/False; update the stub
signature for remove_event_watcher(watcher: T, /) to return bool (i.e., -> bool)
so the type matches the C function's success/failure boolean result; ensure the
change mirrors the existing add_event_watcher signature style and keep the
generic T parameter unchanged.
| * stops returning new types when they are finished, without that | ||
| * test preventing further tests from getting a custom event type.*/ | ||
| _custom_event = _PGE_CUSTOM_EVENT_INIT; | ||
| Py_DECREF(_pg_event_watcher_list); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check pygame quit order to see if SDL is quit before event module cleanup
rg -n "AutoQuit|SDL_Quit" --type c -C 3 src_c/Repository: pygame-community/pygame-ce
Length of output: 3015
🏁 Script executed:
# Get full pgEvent_AutoQuit function
sed -n '806,830p' src_c/event.c | cat -n
# Get full base.c quit context
sed -n '260,270p' src_c/base.c | cat -n
# Search for how module quits are registered/ordered
rg -n "pg_mod_autoquit|_internal_mod_quit" --type c -B 2 -A 2 src_c/ | head -50Repository: pygame-community/pygame-ce
Length of output: 3820
🏁 Script executed:
# Find SDL_AddEventWatch usage in event.c
rg -n "SDL_AddEventWatch|SDL_RemoveEventWatch" --type c src_c/
# Look at _pg_event_watcher_list usage
rg -n "_pg_event_watcher_list" --type c src_c/ -B 2 -A 2 | head -80
# Find where watchers are added to understand the structure
rg -n "PyList_Append.*watcher" --type c src_c/ -B 3 -A 3Repository: pygame-community/pygame-ce
Length of output: 2713
🏁 Script executed:
# Find pg_watcher_wrapper implementation
rg -n "pg_watcher_wrapper" --type c -B 5 -A 15 src_c/event.c | head -60
# Check if there's any cleanup loop that removes watchers before quit
rg -n "for.*_pg_event_watcher_list|while.*_pg_event_watcher_list" --type c src_c/Repository: pygame-community/pygame-ce
Length of output: 2081
🏁 Script executed:
# Verify if SDL_Quit implicitly removes event watches by checking SDL documentation behavior
# and if there's any existing cleanup loop we missed
# Search for any cleanup before DECREF in pgEvent_AutoQuit
sed -n '806,825p' src_c/event.c | cat -n
# Check if there's similar pattern elsewhere (e.g., in pg_event_remove_watcher shows the right pattern)
sed -n '2708,2740p' src_c/event.c | cat -nRepository: pygame-community/pygame-ce
Length of output: 2063
Remove event watches explicitly before releasing watcher list to prevent use-after-free.
The watcher list is DECREF'd without first removing the SDL event watches, but pg_watcher_wrapper will dereference these Python objects if SDL delivers events between DECREF and SDL_Quit. The proper pattern (used in pg_event_remove_watcher) is to call SDL_RemoveEventWatch(pg_watcher_wrapper, watcher) for each watcher in the list before DECREFing it.
Add a cleanup loop in pgEvent_AutoQuit before the Py_DECREF(_pg_event_watcher_list) call that iterates through the watcher list and unregisters each watcher with SDL.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src_c/event.c` at line 823, In pgEvent_AutoQuit, before the existing
Py_DECREF(_pg_event_watcher_list) call, iterate over each entry in
_pg_event_watcher_list and call SDL_RemoveEventWatch(pg_watcher_wrapper,
watcher) to unregister it (same pattern used by pg_event_remove_watcher) so SDL
won't call pg_watcher_wrapper after the Python objects are DECREF'd; then
proceed to Py_DECREF(_pg_event_watcher_list) and SDL_Quit as before.
| int proxy_frees_on_end = dict_proxy->do_free_at_end; | ||
| dict_proxy->do_free_at_end = false; | ||
| SDL_AtomicUnlock(&dict_proxy->lock); | ||
| // Contruct the event object | ||
| PyObject *eventObj = pgEvent_New(event); | ||
| // Restore the state of do_free_at_end | ||
| dict_proxy->do_free_at_end = proxy_frees_on_end; | ||
| return eventObj; |
There was a problem hiding this comment.
Unlocked write to do_free_at_end after releasing lock.
Line 2627 writes to dict_proxy->do_free_at_end without holding the lock, creating a data race with other threads that may access this field. While the incremented num_on_queue prevents premature freeing, this is still technically undefined behavior under C11 memory model.
Consider re-acquiring the lock for the restore:
🔒 Proposed fix
SDL_AtomicUnlock(&dict_proxy->lock);
// Contruct the event object
PyObject *eventObj = pgEvent_New(event);
+ SDL_AtomicLock(&dict_proxy->lock);
// Restore the state of do_free_at_end
dict_proxy->do_free_at_end = proxy_frees_on_end;
+ SDL_AtomicUnlock(&dict_proxy->lock);
return eventObj;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src_c/event.c` around lines 2621 - 2628, Release the lock before calling
pgEvent_New is fine, but the write to dict_proxy->do_free_at_end must be
protected: after creating eventObj with pgEvent_New(event) re-acquire the same
lock (SDL_AtomicLock(&dict_proxy->lock)), restore dict_proxy->do_free_at_end
from proxy_frees_on_end, then unlock with SDL_AtomicUnlock(&dict_proxy->lock);
this ensures the write is synchronized while still allowing the lock to be
released during the potentially blocking pgEvent_New call; keep num_on_queue
behavior unchanged.
| if (PyErr_Occurred()) { | ||
| Py_XDECREF(returnValue); | ||
| PyGILState_Release(gstate); | ||
| _pg_deferred_error = PyErr_GetRaisedException(); | ||
| Py_INCREF(_pg_deferred_error); | ||
| PyErr_Clear(); | ||
| return 0; |
There was a problem hiding this comment.
Reference leak: extra Py_INCREF on deferred error.
PyErr_GetRaisedException() at line 2671 returns a new reference (steals from thread state). The Py_INCREF at line 2672 adds another reference. Later, PyErr_SetRaisedException steals only one reference, leaving an unreleased reference and causing a memory leak.
🐛 Proposed fix
if (PyErr_Occurred()) {
Py_XDECREF(returnValue);
PyGILState_Release(gstate);
_pg_deferred_error = PyErr_GetRaisedException();
- Py_INCREF(_pg_deferred_error);
PyErr_Clear();
return 0;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src_c/event.c` around lines 2668 - 2674, The deferred-error path in
src_c/event.c is leaking a reference because PyErr_GetRaisedException() already
returns a new reference which is being incremented again by
Py_INCREF(_pg_deferred_error); remove the extra Py_INCREF call so
_pg_deferred_error holds the single new reference returned by
PyErr_GetRaisedException(), and ensure later code that calls
PyErr_SetRaisedException consumes/steals exactly one reference as intended
(references involved: PyErr_GetRaisedException, _pg_deferred_error,
PyErr_SetRaisedException).
| def test_remove_event_watcher(self): | ||
| pygame.event.clear() | ||
| """Check that the event watcher is removed""" | ||
| counter = 0 | ||
|
|
||
| def eventWatcher(event): | ||
| nonlocal counter | ||
| counter += 1 | ||
|
|
||
| pygame.event.add_event_watcher(eventWatcher) | ||
| pygame.event.remove_event_watcher(eventWatcher) | ||
|
|
||
| self.assertEqual(counter, 0) | ||
|
|
||
| pygame.event.clear() | ||
| pygame.event.post(pygame.event.Event(pygame.VIDEOEXPOSE)) | ||
|
|
||
| pygame.event.poll() # Make sure that SDL notices | ||
|
|
||
| self.assertEqual(counter, 0) |
There was a problem hiding this comment.
Misplaced docstring and missing cleanup.
The docstring on line 979 appears after pygame.event.clear(), so Python treats it as a string literal, not documentation. Move it immediately after the def line:
def test_remove_event_watcher(self):
+ """Check that the event watcher is removed"""
pygame.event.clear()
- """Check that the event watcher is removed"""
counter = 0Additionally, consider adding explicit watcher cleanup at the end of the test to ensure the watcher is removed even if it wasn't successfully removed by the function under test.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_remove_event_watcher(self): | |
| pygame.event.clear() | |
| """Check that the event watcher is removed""" | |
| counter = 0 | |
| def eventWatcher(event): | |
| nonlocal counter | |
| counter += 1 | |
| pygame.event.add_event_watcher(eventWatcher) | |
| pygame.event.remove_event_watcher(eventWatcher) | |
| self.assertEqual(counter, 0) | |
| pygame.event.clear() | |
| pygame.event.post(pygame.event.Event(pygame.VIDEOEXPOSE)) | |
| pygame.event.poll() # Make sure that SDL notices | |
| self.assertEqual(counter, 0) | |
| def test_remove_event_watcher(self): | |
| """Check that the event watcher is removed""" | |
| pygame.event.clear() | |
| counter = 0 | |
| def eventWatcher(event): | |
| nonlocal counter | |
| counter += 1 | |
| pygame.event.add_event_watcher(eventWatcher) | |
| pygame.event.remove_event_watcher(eventWatcher) | |
| self.assertEqual(counter, 0) | |
| pygame.event.clear() | |
| pygame.event.post(pygame.event.Event(pygame.VIDEOEXPOSE)) | |
| pygame.event.poll() # Make sure that SDL notices | |
| self.assertEqual(counter, 0) |
🧰 Tools
🪛 Ruff (0.15.4)
[warning] 982-982: Unused function argument: event
(ARG001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/event_test.py` around lines 977 - 996, The test_remove_event_watcher
docstring is misplaced after pygame.event.clear() so move the triple-quoted
string immediately below the def test_remove_event_watcher(self): line to make
it a proper docstring; also add explicit cleanup to guarantee watcher removal
(call pygame.event.remove_event_watcher(eventWatcher) in a finally block or
after assertions) so that eventWatcher is always unregistered even if the test
fails—refer to the eventWatcher function and the pygame.event.add_event_watcher
/ pygame.event.remove_event_watcher calls to locate where to add the cleanup.
3 participants
Adds
pygame.event.add_event_watcher,pygame.event.remove_event_watcher,pygame.event.add_event_filter, andpygame.event.remove_event_filteras loose wrappers around the corresponding SDL functions as a potential new way to handle events.This has two primary rationales.
Firstly, to allow using an event handler paradigm if desired while having no impact on the standard event loop structure.
As this PR does nothing without explicitly opting into it opens up more options while not having any impact on other methods of handling events.
Secondarily, event watchers can be used as an effective solution for the quirk of the Windows version of SDL of the main thread freezing while the OS is sending resize or window movement events. This allows for more advanced users who may be impacted by the issue to resolve it without having to mess with the windows APIs directly. An example of the issue from upstream pygame can be found here.
A simple example that will print out all events that are added to the queue could be done like this:
and an example of a filter that will block only left click events:
I have also created a more complex demo implementation showcasing the event watchers as well as demonstrating both the windows issue and an example solution for the issue here.