feat: Exit on first Python callback exception (closes #133)
By default, McRogueFace now exits with code 1 on the first unhandled exception in timer, click, key, or animation callbacks. This prevents repeated exception output that wastes resources in AI-driven development. Changes: - Add exit_on_exception config flag (default: true) - Add --continue-after-exceptions CLI flag to preserve old behavior - Update exception handlers in Timer, PyCallable, and Animation - Signal game loop via McRFPy_API atomic flags - Return proper exit code from main() Before: Timer exceptions repeated 1000+ times until timeout After: Single traceback, clean exit with code 1 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
parent
9028bf485e
commit
19ded088b0
|
|
@ -3,6 +3,7 @@
|
||||||
#include "UIEntity.h"
|
#include "UIEntity.h"
|
||||||
#include "PyAnimation.h"
|
#include "PyAnimation.h"
|
||||||
#include "McRFPy_API.h"
|
#include "McRFPy_API.h"
|
||||||
|
#include "GameEngine.h"
|
||||||
#include "PythonObjectCache.h"
|
#include "PythonObjectCache.h"
|
||||||
#include <cmath>
|
#include <cmath>
|
||||||
#include <algorithm>
|
#include <algorithm>
|
||||||
|
|
@ -368,9 +369,14 @@ void Animation::triggerCallback() {
|
||||||
Py_DECREF(args);
|
Py_DECREF(args);
|
||||||
|
|
||||||
if (!result) {
|
if (!result) {
|
||||||
// Print error but don't crash
|
std::cerr << "Animation callback raised an exception:" << std::endl;
|
||||||
PyErr_Print();
|
PyErr_Print();
|
||||||
PyErr_Clear(); // Clear the error state
|
PyErr_Clear();
|
||||||
|
|
||||||
|
// Check if we should exit on exception
|
||||||
|
if (McRFPy_API::game && McRFPy_API::game->getConfig().exit_on_exception) {
|
||||||
|
McRFPy_API::signalPythonException();
|
||||||
|
}
|
||||||
} else {
|
} else {
|
||||||
Py_DECREF(result);
|
Py_DECREF(result);
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -121,6 +121,12 @@ CommandLineParser::ParseResult CommandLineParser::parse(McRogueFaceConfig& confi
|
||||||
current_arg++;
|
current_arg++;
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if (arg == "--continue-after-exceptions") {
|
||||||
|
config.exit_on_exception = false;
|
||||||
|
current_arg++;
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
|
||||||
// If no flags matched, treat as positional argument (script name)
|
// If no flags matched, treat as positional argument (script name)
|
||||||
if (arg[0] != '-') {
|
if (arg[0] != '-') {
|
||||||
|
|
@ -160,6 +166,8 @@ void CommandLineParser::print_help() {
|
||||||
<< " --audio-off : disable audio\n"
|
<< " --audio-off : disable audio\n"
|
||||||
<< " --audio-on : enable audio (even in headless mode)\n"
|
<< " --audio-on : enable audio (even in headless mode)\n"
|
||||||
<< " --screenshot [path] : take a screenshot in headless mode\n"
|
<< " --screenshot [path] : take a screenshot in headless mode\n"
|
||||||
|
<< " --continue-after-exceptions : don't exit on Python callback exceptions\n"
|
||||||
|
<< " (default: exit on first exception)\n"
|
||||||
<< "\n"
|
<< "\n"
|
||||||
<< "Arguments:\n"
|
<< "Arguments:\n"
|
||||||
<< " file : program read from script file\n"
|
<< " file : program read from script file\n"
|
||||||
|
|
|
||||||
|
|
@ -289,6 +289,11 @@ void GameEngine::run()
|
||||||
if (config.auto_exit_after_exec && timers.empty()) {
|
if (config.auto_exit_after_exec && timers.empty()) {
|
||||||
running = false;
|
running = false;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Check if a Python exception has signaled exit
|
||||||
|
if (McRFPy_API::shouldExit()) {
|
||||||
|
running = false;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Clean up before exiting the run loop
|
// Clean up before exiting the run loop
|
||||||
|
|
|
||||||
|
|
@ -153,6 +153,7 @@ public:
|
||||||
std::shared_ptr<Timer> getTimer(const std::string& name);
|
std::shared_ptr<Timer> getTimer(const std::string& name);
|
||||||
void setWindowScale(float);
|
void setWindowScale(float);
|
||||||
bool isHeadless() const { return headless; }
|
bool isHeadless() const { return headless; }
|
||||||
|
const McRogueFaceConfig& getConfig() const { return config; }
|
||||||
void processEvent(const sf::Event& event);
|
void processEvent(const sf::Event& event);
|
||||||
|
|
||||||
// Window property accessors
|
// Window property accessors
|
||||||
|
|
|
||||||
|
|
@ -27,6 +27,9 @@ std::shared_ptr<PyFont> McRFPy_API::default_font;
|
||||||
std::shared_ptr<PyTexture> McRFPy_API::default_texture;
|
std::shared_ptr<PyTexture> McRFPy_API::default_texture;
|
||||||
PyObject* McRFPy_API::mcrf_module;
|
PyObject* McRFPy_API::mcrf_module;
|
||||||
|
|
||||||
|
// Exception handling state
|
||||||
|
std::atomic<bool> McRFPy_API::exception_occurred{false};
|
||||||
|
std::atomic<int> McRFPy_API::exit_code{0};
|
||||||
|
|
||||||
static PyMethodDef mcrfpyMethods[] = {
|
static PyMethodDef mcrfpyMethods[] = {
|
||||||
|
|
||||||
|
|
@ -1144,6 +1147,23 @@ PyObject* McRFPy_API::_getMetrics(PyObject* self, PyObject* args) {
|
||||||
// Add general metrics
|
// Add general metrics
|
||||||
PyDict_SetItemString(dict, "current_frame", PyLong_FromLong(game->getFrame()));
|
PyDict_SetItemString(dict, "current_frame", PyLong_FromLong(game->getFrame()));
|
||||||
PyDict_SetItemString(dict, "runtime", PyFloat_FromDouble(game->runtime.getElapsedTime().asSeconds()));
|
PyDict_SetItemString(dict, "runtime", PyFloat_FromDouble(game->runtime.getElapsedTime().asSeconds()));
|
||||||
|
|
||||||
return dict;
|
return dict;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Exception handling implementation
|
||||||
|
void McRFPy_API::signalPythonException() {
|
||||||
|
// Check if we should exit on exception (consult config via game)
|
||||||
|
if (game && !game->isHeadless()) {
|
||||||
|
// In windowed mode, respect the config setting
|
||||||
|
// Access config through game engine - but we need to check the config
|
||||||
|
}
|
||||||
|
|
||||||
|
// For now, always signal - the game loop will check the config
|
||||||
|
exception_occurred.store(true);
|
||||||
|
exit_code.store(1);
|
||||||
|
}
|
||||||
|
|
||||||
|
bool McRFPy_API::shouldExit() {
|
||||||
|
return exception_occurred.load();
|
||||||
|
}
|
||||||
|
|
|
||||||
|
|
@ -2,6 +2,7 @@
|
||||||
#include "Common.h"
|
#include "Common.h"
|
||||||
#include "Python.h"
|
#include "Python.h"
|
||||||
#include <list>
|
#include <list>
|
||||||
|
#include <atomic>
|
||||||
|
|
||||||
#include "PyFont.h"
|
#include "PyFont.h"
|
||||||
#include "PyTexture.h"
|
#include "PyTexture.h"
|
||||||
|
|
@ -85,4 +86,10 @@ public:
|
||||||
static void triggerSceneChange(const std::string& from_scene, const std::string& to_scene);
|
static void triggerSceneChange(const std::string& from_scene, const std::string& to_scene);
|
||||||
static void updatePythonScenes(float dt);
|
static void updatePythonScenes(float dt);
|
||||||
static void triggerResize(int width, int height);
|
static void triggerResize(int width, int height);
|
||||||
|
|
||||||
|
// Exception handling - signal game loop to exit on unhandled Python exceptions
|
||||||
|
static std::atomic<bool> exception_occurred;
|
||||||
|
static std::atomic<int> exit_code;
|
||||||
|
static void signalPythonException(); // Called by exception handlers
|
||||||
|
static bool shouldExit(); // Checked by game loop
|
||||||
};
|
};
|
||||||
|
|
|
||||||
|
|
@ -31,6 +31,10 @@ struct McRogueFaceConfig {
|
||||||
|
|
||||||
// Auto-exit when no timers remain (for --headless --exec automation)
|
// Auto-exit when no timers remain (for --headless --exec automation)
|
||||||
bool auto_exit_after_exec = false;
|
bool auto_exit_after_exec = false;
|
||||||
|
|
||||||
|
// Exception handling: exit on first Python callback exception (default: true)
|
||||||
|
// Use --continue-after-exceptions to disable
|
||||||
|
bool exit_on_exception = true;
|
||||||
};
|
};
|
||||||
|
|
||||||
#endif // MCROGUEFACE_CONFIG_H
|
#endif // MCROGUEFACE_CONFIG_H
|
||||||
|
|
@ -1,4 +1,6 @@
|
||||||
#include "PyCallable.h"
|
#include "PyCallable.h"
|
||||||
|
#include "McRFPy_API.h"
|
||||||
|
#include "GameEngine.h"
|
||||||
|
|
||||||
PyCallable::PyCallable(PyObject* _target)
|
PyCallable::PyCallable(PyObject* _target)
|
||||||
{
|
{
|
||||||
|
|
@ -51,9 +53,14 @@ void PyClickCallable::call(sf::Vector2f mousepos, std::string button, std::strin
|
||||||
PyObject* retval = PyCallable::call(args, NULL);
|
PyObject* retval = PyCallable::call(args, NULL);
|
||||||
if (!retval)
|
if (!retval)
|
||||||
{
|
{
|
||||||
std::cout << "ClickCallable has raised an exception. It's going to STDERR and being dropped:" << std::endl;
|
std::cerr << "Click callback raised an exception:" << std::endl;
|
||||||
PyErr_Print();
|
PyErr_Print();
|
||||||
PyErr_Clear();
|
PyErr_Clear();
|
||||||
|
|
||||||
|
// Check if we should exit on exception
|
||||||
|
if (McRFPy_API::game && McRFPy_API::game->getConfig().exit_on_exception) {
|
||||||
|
McRFPy_API::signalPythonException();
|
||||||
|
}
|
||||||
} else if (retval != Py_None)
|
} else if (retval != Py_None)
|
||||||
{
|
{
|
||||||
std::cout << "ClickCallable returned a non-None value. It's not an error, it's just not being saved or used." << std::endl;
|
std::cout << "ClickCallable returned a non-None value. It's not an error, it's just not being saved or used." << std::endl;
|
||||||
|
|
@ -81,9 +88,14 @@ void PyKeyCallable::call(std::string key, std::string action)
|
||||||
PyObject* retval = PyCallable::call(args, NULL);
|
PyObject* retval = PyCallable::call(args, NULL);
|
||||||
if (!retval)
|
if (!retval)
|
||||||
{
|
{
|
||||||
std::cout << "KeyCallable has raised an exception. It's going to STDERR and being dropped:" << std::endl;
|
std::cerr << "Key callback raised an exception:" << std::endl;
|
||||||
PyErr_Print();
|
PyErr_Print();
|
||||||
PyErr_Clear();
|
PyErr_Clear();
|
||||||
|
|
||||||
|
// Check if we should exit on exception
|
||||||
|
if (McRFPy_API::game && McRFPy_API::game->getConfig().exit_on_exception) {
|
||||||
|
McRFPy_API::signalPythonException();
|
||||||
|
}
|
||||||
} else if (retval != Py_None)
|
} else if (retval != Py_None)
|
||||||
{
|
{
|
||||||
std::cout << "KeyCallable returned a non-None value. It's not an error, it's just not being saved or used." << std::endl;
|
std::cout << "KeyCallable returned a non-None value. It's not an error, it's just not being saved or used." << std::endl;
|
||||||
|
|
|
||||||
|
|
@ -1,6 +1,8 @@
|
||||||
#include "Timer.h"
|
#include "Timer.h"
|
||||||
#include "PythonObjectCache.h"
|
#include "PythonObjectCache.h"
|
||||||
#include "PyCallable.h"
|
#include "PyCallable.h"
|
||||||
|
#include "McRFPy_API.h"
|
||||||
|
#include "GameEngine.h"
|
||||||
|
|
||||||
Timer::Timer(PyObject* _target, int _interval, int now, bool _once)
|
Timer::Timer(PyObject* _target, int _interval, int now, bool _once)
|
||||||
: callback(std::make_shared<PyCallable>(_target)), interval(_interval), last_ran(now),
|
: callback(std::make_shared<PyCallable>(_target)), interval(_interval), last_ran(now),
|
||||||
|
|
@ -51,10 +53,15 @@ bool Timer::test(int now)
|
||||||
Py_DECREF(args);
|
Py_DECREF(args);
|
||||||
|
|
||||||
if (!retval)
|
if (!retval)
|
||||||
{
|
{
|
||||||
std::cout << "Timer callback has raised an exception. It's going to STDERR and being dropped:" << std::endl;
|
std::cerr << "Timer callback raised an exception:" << std::endl;
|
||||||
PyErr_Print();
|
PyErr_Print();
|
||||||
PyErr_Clear();
|
PyErr_Clear();
|
||||||
|
|
||||||
|
// Check if we should exit on exception
|
||||||
|
if (McRFPy_API::game && McRFPy_API::game->getConfig().exit_on_exception) {
|
||||||
|
McRFPy_API::signalPythonException();
|
||||||
|
}
|
||||||
} else if (retval != Py_None)
|
} else if (retval != Py_None)
|
||||||
{
|
{
|
||||||
std::cout << "Timer returned a non-None value. It's not an error, it's just not being saved or used." << std::endl;
|
std::cout << "Timer returned a non-None value. It's not an error, it's just not being saved or used." << std::endl;
|
||||||
|
|
|
||||||
15
src/main.cpp
15
src/main.cpp
|
|
@ -44,6 +44,11 @@ int run_game_engine(const McRogueFaceConfig& config)
|
||||||
if (Py_IsInitialized()) {
|
if (Py_IsInitialized()) {
|
||||||
McRFPy_API::api_shutdown();
|
McRFPy_API::api_shutdown();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Return exception exit code if a Python exception signaled exit
|
||||||
|
if (McRFPy_API::shouldExit()) {
|
||||||
|
return McRFPy_API::exit_code.load();
|
||||||
|
}
|
||||||
return 0;
|
return 0;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -181,9 +186,13 @@ int run_python_interpreter(const McRogueFaceConfig& config, int argc, char* argv
|
||||||
|
|
||||||
// Run the game engine after script execution
|
// Run the game engine after script execution
|
||||||
engine->run();
|
engine->run();
|
||||||
|
|
||||||
McRFPy_API::api_shutdown();
|
McRFPy_API::api_shutdown();
|
||||||
delete engine;
|
delete engine;
|
||||||
|
// Return exception exit code if signaled
|
||||||
|
if (McRFPy_API::shouldExit()) {
|
||||||
|
return McRFPy_API::exit_code.load();
|
||||||
|
}
|
||||||
return result;
|
return result;
|
||||||
}
|
}
|
||||||
else if (config.interactive_mode) {
|
else if (config.interactive_mode) {
|
||||||
|
|
@ -207,6 +216,10 @@ int run_python_interpreter(const McRogueFaceConfig& config, int argc, char* argv
|
||||||
engine->run();
|
engine->run();
|
||||||
McRFPy_API::api_shutdown();
|
McRFPy_API::api_shutdown();
|
||||||
delete engine;
|
delete engine;
|
||||||
|
// Return exception exit code if signaled
|
||||||
|
if (McRFPy_API::shouldExit()) {
|
||||||
|
return McRFPy_API::exit_code.load();
|
||||||
|
}
|
||||||
return 0;
|
return 0;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -0,0 +1,31 @@
|
||||||
|
#!/usr/bin/env python3
|
||||||
|
"""Test for --continue-after-exceptions behavior (Issue #133)
|
||||||
|
|
||||||
|
This test verifies that:
|
||||||
|
1. By default, unhandled exceptions in timer callbacks cause immediate exit with code 1
|
||||||
|
2. With --continue-after-exceptions, exceptions are logged but execution continues
|
||||||
|
"""
|
||||||
|
|
||||||
|
import mcrfpy
|
||||||
|
import sys
|
||||||
|
|
||||||
|
def timer_that_raises(runtime):
|
||||||
|
"""A timer callback that raises an exception"""
|
||||||
|
raise ValueError("Intentional test exception")
|
||||||
|
|
||||||
|
# Create a test scene
|
||||||
|
mcrfpy.createScene("test")
|
||||||
|
mcrfpy.setScene("test")
|
||||||
|
|
||||||
|
# Schedule the timer - it will fire after 50ms
|
||||||
|
mcrfpy.setTimer("raise_exception", timer_that_raises, 50)
|
||||||
|
|
||||||
|
# This test expects:
|
||||||
|
# - Default behavior: exit with code 1 after first exception
|
||||||
|
# - With --continue-after-exceptions: continue running (would need timeout or explicit exit)
|
||||||
|
#
|
||||||
|
# The test runner should:
|
||||||
|
# 1. Run without --continue-after-exceptions and expect exit code 1
|
||||||
|
# 2. Run with --continue-after-exceptions and expect it to not exit immediately
|
||||||
|
|
||||||
|
print("Test initialized - timer will raise exception in 50ms")
|
||||||
|
|
@ -0,0 +1,38 @@
|
||||||
|
#!/bin/bash
|
||||||
|
# Manual test for --continue-after-exceptions feature (Issue #133)
|
||||||
|
#
|
||||||
|
# This test must be run manually because it verifies exit codes
|
||||||
|
# rather than test output.
|
||||||
|
|
||||||
|
echo "Testing --continue-after-exceptions feature..."
|
||||||
|
echo
|
||||||
|
|
||||||
|
cd "$(dirname "$0")/../../build"
|
||||||
|
|
||||||
|
# Test 1: Default behavior - should exit with code 1 on first exception
|
||||||
|
echo "Test 1: Default behavior (exit on first exception)"
|
||||||
|
timeout 5 ./mcrogueface --headless --exec ../tests/notes/test_exception_exit.py 2>&1
|
||||||
|
EXIT_CODE=$?
|
||||||
|
echo "Exit code: $EXIT_CODE"
|
||||||
|
if [ $EXIT_CODE -eq 1 ]; then
|
||||||
|
echo "[PASS] Exit code is 1 as expected"
|
||||||
|
else
|
||||||
|
echo "[FAIL] Expected exit code 1, got $EXIT_CODE"
|
||||||
|
exit 1
|
||||||
|
fi
|
||||||
|
echo
|
||||||
|
|
||||||
|
# Test 2: --continue-after-exceptions - should keep running until timeout
|
||||||
|
echo "Test 2: --continue-after-exceptions (continue after exception)"
|
||||||
|
timeout 1 ./mcrogueface --headless --continue-after-exceptions --exec ../tests/notes/test_exception_exit.py 2>&1 | tail -5
|
||||||
|
EXIT_CODE=${PIPESTATUS[0]}
|
||||||
|
echo "Exit code: $EXIT_CODE"
|
||||||
|
if [ $EXIT_CODE -eq 124 ]; then
|
||||||
|
echo "[PASS] Timeout killed it (exit code 124) - continued running as expected"
|
||||||
|
else
|
||||||
|
echo "[FAIL] Expected exit code 124 (timeout), got $EXIT_CODE"
|
||||||
|
exit 1
|
||||||
|
fi
|
||||||
|
echo
|
||||||
|
|
||||||
|
echo "All tests PASSED!"
|
||||||
Loading…
Reference in New Issue