Skip to content

Fix cases where startup errors can cause crashes.#600

Merged
bsteffensmeier merged 2 commits into
ninia:dev_4.3from
bsteffensmeier:startup-errors
Mar 14, 2025
Merged

Fix cases where startup errors can cause crashes.#600
bsteffensmeier merged 2 commits into
ninia:dev_4.3from
bsteffensmeier:startup-errors

Conversation

@bsteffensmeier

Copy link
Copy Markdown
Member

Using invalid SubInterpreterOptions was causing a crash rather than throwing an exception even though we have checks to throw an exception. It turns out we were trying to throw a JepException before initializing the variable that holds JepException is initialized. We have an existing function to handle startup errors before JepException is available but that function checks for python exceptions and errors during python initialization probably shouldn't use the python APIs. This change makes the python error check optional in handle_startup_exception and changes any startup errors that use THROW_JEP to use handle_startup_exception instead.

@bsteffensmeier bsteffensmeier requested a review from ndjensen March 13, 2025 01:50

@ndjensen ndjensen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good improvements!

Comment thread src/main/c/Jep/pyembed.c Outdated
PyObject *modjep = PyDict_GetItemString(sysmodules, "_jep");
if (!modjep && PyErr_Occurred()) {
handle_startup_exception(env, "Error checking for exisitng module _jep");
handle_startup_exception(env, "Error checking for exisitng module _jep", 1);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Existing problem but should be fixed, the spelling of "existing".

@bsteffensmeier bsteffensmeier merged commit da52c9e into ninia:dev_4.3 Mar 14, 2025
@bsteffensmeier bsteffensmeier deleted the startup-errors branch March 14, 2025 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants