Created attachment 159162 [details] [review]
Print any error messages raised inside _pygi_closure_handle

Review of attachment 159162 [details] [review]:

Hm.... I don't immediately see any reason not to do this.  Perhaps though we should surround the prints with  PyErr_Occurred() first though just to be sure?

(In reply to comment #2)
> Review of attachment 159162 [details] [review]:
> 
> Hm.... I don't immediately see any reason not to do this.  Perhaps though we
> should surround the prints with  PyErr_Occurred() first though just to be sure?

I have checked and AFAICS all those functions should be raising an exception when returning a failure value. But sure, we can add it if needed.

What is this patch based on? I could not apply it to pygi master to add the PyErr_Occured checks.

Created attachment 159407 [details] [review]
Print any error messages raised inside _pygi_closure_handle

Rebased on top of HEAD

Review of attachment 159407 [details] [review]:

Looks good. PyErr_Print will raise a fatal error if an exception is not set, so it'll be easy to find bugs related to wrong usage of it.

Attachment 159407 [details] pushed as 24bb89f - Print any error messages raised inside _pygi_closure_handle

This patch breaks the callback tests.  The callback tests were not strict enough.  The following patch fixes the tests and shows that this printing patch is not good.

Created attachment 159532 [details] [review]
Make the tests a bit stricter

(In reply to comment #8)
> This patch breaks the callback tests.  The callback tests were not strict
> enough.  The following patch fixes the tests and shows that this printing patch
> is not good.

I'm confused at those tests, I thought for callbacks we just wanted to print an error to stdout? This is what PyGTK and other static bindings do.

Also, what would happen if the callback is called outside any python context?

Or if the error happens in a python callback that was called by C code that was invoked from a Python callback? (This happens a lot in async-heavy code such in telepathy or dbus)

Also, I think it's unsafe to leave a Python exception set when we leave from a Python execution context, because we cannot know which Python code will be executed next.

(In reply to comment #8)
> This patch breaks the callback tests.  The callback tests were not strict
> enough.  The following patch fixes the tests and shows that this printing patch
> is not good.

That's unfortunately expected. Exceptions are not propagated over callback boundaries. I think you can just remove the tests that check for that.

You could alternatively fix so that exceptions are properly propagated, http://stackoverflow.com/questions/541329/is-it-possible-to-programmatically-construct-a-python-stack-frame-and-start-execu/ mentions how to do it.

It's a nice feature to have, it should be implemented soon since it'll be part of the API, eg we won't be able to implement it after applications start to depend on PyGI.

I agree that in an ideal world we would work to make exception propegation behave appropriatly, but we need to think long and hard about the pygtk api break by doing that.

Comment on attachment 159532 [details] [review]
Make the tests a bit stricter

Taking this patch out from the review queue because we cannot accept it until we have proper error reporting.

I agree to Johan in comment 12. The current behaviour is now established, and it is not necessarily desirable (or safe) to propagate callback exceptions back to the original caller. So I committed

  http://git.gnome.org/browse/pygobject/commit/?id=903283119896f3e054694484da4147788b02ce60

now to clarify this. Re-closing, the original patch for printing the exception went in long ago and that's about as far as we can go with this in my opinion.

(In reply to comment #15)
> I agree to Johan in comment 12. The current behaviour is now established, and
> it is not necessarily desirable (or safe) to propagate callback exceptions back
> to the original caller. So I committed
> 
>  
> http://git.gnome.org/browse/pygobject/commit/?id=903283119896f3e054694484da4147788b02ce60
> 
> now to clarify this. Re-closing, the original patch for printing the exception
> went in long ago and that's about as far as we can go with this in my opinion.

I would have liked to see this fixed (along with https://bugzilla.gnome.org/show_bug.cgi?id=575652). As an app-developer, having a full cross boundary exception mechanism is important. I think it will become even more important as more software is written using gi and different languages are interacting with shared components. Being able to know when an error occurred and present that to a user in a consistent way (at least giving them the ability to report the error to the dev) should be possible. With only printing the exception this is not possible, especially if apps are launched through a launcher, without a terminal the errors will be silent and users will not know what is going on.

A solution would be to catch any python errors (as is happening now) and translate them into glib errors as best as possible. This way other languages can gather some type of meaning from the python error. In addition to this, the python error should be cleared and a reference stashed in the gi package for later use, sort of a "last python gi error" attribute(s). If the app is in a different language but calling python components where the error occurred, then the stashed exception is never used and those languages can use the glib error that was set. However, if control is returned to python, then these stashed errors can be set back on the python context generating the exact exception which originally occurred.

Proper behavior could initially be made optional and rolled into a specific release as default (similar to how python uses future). I can take a stab at doing this work if you guys re-consider.

Translating an exception in the callback to a g_error() does sound like a nice idea. That should avoid the Python <-> C <-> Python boundaries.

Reopening, as you said you would like to work on this.

Upon further thinking of the problem in the context of Gtk, my proposed solution is not valuable in regards to arbitrary errors in widget callbacks. Since we don't want the exception to break out of the main loop. But rather we just want a notification when an error occurred in these cases, button clicked for example. I've used sys.excepthook before but I didn't realize this is called from PyErr_Print. So basically this already works. Adding a sys.excepthook will not print anything when an exception occurs within a callback, but it will call the custom except hook giving it the exception details and call stack. This at least solves my immediate concerns but is still less than ideal for all cases.

The case described in https://bugzilla.gnome.org/show_bug.cgi?id=575652 
is still valid and are not necessarily help by sys.excepthook
This along with a direct invocation of a python handler through c back to an outer python context:
>>> btn.notify('clicked')
should still raise in the current context if the clicked handler raised.

I had assumed g_error was something that is set globally rather then passed around. Unfortunately it doesn't look like connected handlers get passed a g_error to fill out when they are invoked. A quick look at the gobject C library and there is not really any error handling support. There is also no reason a global error system could't be added to gobject as i think it would be highly beneficial for cross language support. If anyone with more experience has ideas, please let me know. Thanks.

Comment on attachment 159532 [details] [review]
Make the tests a bit stricter

Unmarking as patch to get it off the patch review queue.

Comment on attachment 159532 [details] [review]
Make the tests a bit stricter

I'll refine my search instead, sorry for the noise.

*** Bug 575652 has been marked as a duplicate of this bug. ***

*** Bug 694584 has been marked as a duplicate of this bug. ***

At this point, fixing this would break caller expectation and should require explicit enablement. This bug now depends on bug 723736.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/pygobject/issues/6.