Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Python exceptions don't always propagate from an "except +" function in Cython 0.29 #4720

Closed
jngrad opened this issue Apr 7, 2022 · 2 comments

Comments

@jngrad
Copy link

jngrad commented Apr 7, 2022

Describe the bug
When raising a Python exception in a cdef function marked with except +, the exception is not always propagated to the call site. Related to #2603, most likely due to #2615 (comment). Can it be made safe by replacing except + with except *?

To Reproduce
Three files are needed to reproduce the behaviour.

In Makefile:

.DEFAULT_GOAL := main
%.cpp: %.pyx
	python3 -m cython -3 --cplus --directive embedsignature=True --directive binding=True -o $@ $<
%.cpp.o: %.cpp
	g++ -isystem /usr/include/python3.8 -O3 -g -fPIC -pthread -Wall -Wextra -std=c++14 -o $@ -c $<
%.so: %.cpp.o
	g++ -fPIC -O3 -g -pthread -shared -Wl,-soname,helloworld.so -o $@ $<
main.txt: helloworld.so helloworld_utils.so
	touch main.txt
main: main.txt
	python3 -c 'import helloworld;helloworld.MyClass("string");helloworld.MyClass(123);print("No error was raised!")'

In helloworld_utils.pyx:

def to_bytes(s):
    if isinstance(s, unicode):
        return ( < unicode > s).encode("utf8")
    return s

In helloworld.pyx:

import helloworld_utils
from libcpp.string cimport string
from libcpp.unordered_map cimport unordered_map
ctypedef unordered_map[string, string] VarMap

cdef class MyClass:
    def __init__(self, value):
        cdef VarMap converted_value
        converted_value[helloworld_utils.to_bytes("value")] = python_object_to_variant(value)

cdef string python_object_to_variant(value) except +:
    cdef string out
#    if isinstance(value, str):
#        out = helloworld_utils.to_bytes(value)
#        return out
    raise TypeError(f"cannot convert type {type(value)} to std::string")

Running make to build the module and run the python script results in message "No error was raised!" being printed to stdout.

Expected behavior
The expected outcome is a TypeError, and it can be obtained by doing one of the following actions:

  • uncommenting the commented lines in the body of python_object_to_variant()
  • replacing helloworld_utils.to_bytes("value") with b"value" in MyClass.__init__()
  • replacing helloworld_utils.to_bytes("value") with to_bytes("value") in MyClass.__init__() and copy-pasting the definition of to_bytes() in helloworld.pyx
  • replacing except + with except *

The following workarounds didn't help:

  • replacing except + with except +* (due to Bad code generated for except +* function #3065)
  • adding except + to U& operator[](T&) in Cython/Includes/libcpp/unordered_map.pxd
  • replacing converted_value[a] = b by converted_value.insert(pair[string, string](a, b))

Environment:
Reproducible in 2 environments:

  • OS: Ubuntu 20.04 resp. Ubuntu 22.04
  • Python version 3.8.10 resp. 3.9.5
  • Cython version 0.29.14 resp. 0.29.21

Additional context
The bug is always reproducible in the minimal working example provided above, however the bug originally surfaced in a larger Cython project where it happened at random and infrequently. According to Exception handling in cython cdef declarations, declaring the cdef function with except * seems to be the preferred approach in this situation, although it comes with a small overhead. According to #2603, declaring the cdef function with except + shouldn't prevent the propagation of Python exceptions.

@da-woods
Copy link
Contributor

da-woods commented Apr 7, 2022

It isn't completely obvious from the issues you link, but except+ only makes sense for extern functions. Cython functions never raise c++ exceptions and they should never allow c++ exceptions to propagate through them (they only handle reference counting correctly if the c++ exception is caught and handled/converted immediately).

There may be a genuine issue here, but in this case the function should just be except *

@jngrad
Copy link
Author

jngrad commented Apr 7, 2022

@da-woods Thanks for the clarification! It makes sense now. I'll make that change.

kodiakhq bot added a commit to espressomd/espresso that referenced this issue Apr 8, 2022
Fix a regression introduced in #4387 that would randomly block the propagation of a `TypeError` when converting an unsupported Python type to a C++ variant. Please refer to cython/cython#4720 for the details. Restore the corresponding `TypeError` checks in the testsuite.
@jngrad jngrad closed this as completed Apr 8, 2022
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

No branches or pull requests

2 participants