python directors will not work if C++ exceptions are disabled

[blaisegassend]

Currently python directors rely on C++ exceptions to raise various errors that can occur when trying to call a method that is implemented in Python. Code bases that do not allow exceptions can therefore not use python directors, which is unfortunate.

It appears that there are two uses of exceptions:

1. UnknownExceptionHandler.
2. Director*Exception::raise(PyObject *error, const char *msg).

My proposed solution is:

1. Get rid of UnknownExceptionHandler as it is dead code as far as I can tell.
2. Allow the action in Director*Exception::raise to be overridden by a %feature(director:throw).

If anybody sees an issue with this solution let me know. I'll be submitting a patch shortly and would prefer for it not to be controversial once it is written.

[wsfulton]

I just want to make sure you have explored using the "director:except" feature?
The most flexible exception handling is done in Java which uses this feature plus "directorthrows" typemaps. I'd really like the other languages to use the same approach. Can you check that an implementation of this won't meet your needs, I can't say I've given your particular scenario much thought, but it might work. A consistent approach across the different languages (that are relevant) are a priority in accepting patches and so if what I've mentioned does not work, I'd like to understand why not. Some discussion of these were done in #91.

[blaisegassend]

Hi William,

Unfortunately, there are four places where director.swig contains throw/try
statements, which simply cause compilation to fail. So a patch of some sort
is necessary so that those throws can be eliminated when some feature is
activated. Moreover, director:except only overrides one of the places where
the director code throws: when the callee has thrown an exception. Other
places that are not covered are: trying to call a pure method, type
mismatch in the value returned by python. So even with director:except you
can't get rid of all the throwing, and even if it did the code wouldn't
compile because methods containing throw are still present in the c++ file.

I had a quick look at directorthrows, and my understanding is that it is
unrelated to my needs. It seems to provide an alternate to director:except
to handle exceptions that happen in python/java. director:except is already
doing that fine for me. What I need to get rid of is the c++ exceptions
that are being raised when errors happen that are not due to an exception
in the python/java code. Am I understanding that correctly?

I am curious how Java is managing to do directors without c++ exceptions.
How is flow getting from the place where the java code returns to the place
that was called from java without using exceptions? Do you have an example
of a simple java director use in google3 so that I could look at the
generated code?

I'll share a patch in a bit so you can see what I'm up to.

Thanks,
Blaise

On Thu Jan 29 2015 at 12:25:39 PM William S Fulton [email protected]
wrote:

I just want to make sure you have explored using the "director:except"
feature?
The most flexible exception handling is done in Java which uses this
feature plus "directorthrows" typemaps. I'd really like the other languages
to use the same approach. Can you check that an implementation of this
won't meet your needs, I can't say I've given your particular scenario much
thought, but it might work. A consistent approach across the different
languages (that are relevant) are a priority in accepting patches and so if
what I've mentioned does not work, I'd like to understand why not. Some
discussion of these were done in #91
#91.

—
Reply to this email directly or view it on GitHub
#315 (comment).

[ojwb]

Looks like Swig::UnknownExceptionHandler has never actually been really used in SWIG code - it was added in ca7159f with an unused variable dh of this type added to generated wrappers, and then variable dh was removed in 37a08a2.

It's possible it is used in user SWIG interface files. It's undocumented in the manual or CHANGES file though.

The only references in Debian codesearch seem to be in generated wrappers or SWIG itself, which suggests it isn't used in anything packaged for Debian at least:

http://codesearch.debian.net/results/UnknownExceptionHandler/page_1

I think it's pretty safe to remove.

[blaisegassend]

Yeah, my patch just removes it UnknownExceptionHandler as I had concluded
pretty much the same thing as you about its being used. It is the
Director*Exception exceptions that require a bit more work.

Here is what I have right now. I'd love comments. I still need to write
tests to make sure it actually works. This doesn't break the existing tests.
https://paste.googleplex.com/5576946247270400

On Thu Jan 29 2015 at 1:30:32 PM Olly Betts [email protected]
wrote:

Looks like Swig::UnknownExceptionHandler has never actually been really
used in SWIG code - it was added in ca7159f
ca7159f
with an unused variable dh of this type added to generated wrappers, and
then variable dh was removed in 37a08a2
37a08a2
.

It's possible it is used in user SWIG interface files. It's undocumented
in the manual or CHANGES file though.

The only references in Debian codesearch seem to be in generated wrappers
or SWIG itself, which suggests it isn't used in anything packaged for
Debian at least:

http://codesearch.debian.net/results/UnknownExceptionHandler/page_1

I think it's pretty safe to remove.

—
Reply to this email directly or view it on GitHub
#315 (comment).

[ojwb]

That seems to be some sort of internal to google link.

I'd suggest we just drop UnknownExceptionHandler as an initial step - @wsfulton Is there some indirect usage of it we're missing?

[blaisegassend]

Sorry, I missed that this was going outside google. I need to get approval
from our open source folks before I can make the patch public. (Should be a
rubber stamp if my understanding is correct.) I'll ask for permission now.

We can drop it, but there will still be exception use so dropping it isn't
sufficient for my use-case. Tho code won't compile for me without a throw
statement in it.

On Thu Jan 29 2015 at 2:18:44 PM Olly Betts [email protected]
wrote:

That seems to be some sort of internal to google link.

I'd suggest we just drop UnknownExceptionHandler as an initial step -
@wsfulton https://github.com/wsfulton Is there some indirect usage of
it we're missing?

—
Reply to this email directly or view it on GitHub
#315 (comment).

[wsfulton]

On 29 January 2015 at 21:30, Olly Betts [email protected] wrote:

Looks like Swig::UnknownExceptionHandler has never actually been really
used in SWIG code - it was added in ca7159f
ca7159f
with an unused variable dh of this type added to generated wrappers, and
then variable dh was removed in 37a08a2
37a08a2
.

It's possible it is used in user SWIG interface files. It's undocumented
in the manual or CHANGES file though.

The only references in Debian codesearch seem to be in generated wrappers
or SWIG itself, which suggests it isn't used in anything packaged for
Debian at least:

http://codesearch.debian.net/results/UnknownExceptionHandler/page_1

I think it's pretty safe to remove.

Okay, let's do the easy bit first and have a separate patch to remove this
in the first instance.

[wsfulton]

On 29 January 2015 at 20:51, blaisegassend [email protected] wrote:

Hi William,

Unfortunately, there are four places where director.swig contains
throw/try
statements, which simply cause compilation to fail. So a patch of some
sort
is necessary so that those throws can be eliminated when some feature is
activated. Moreover, director:except only overrides one of the places
where
the director code throws: when the callee has thrown an exception. Other
places that are not covered are: trying to call a pure method, type
mismatch in the value returned by python. So even with director:except you
can't get rid of all the throwing, and even if it did the code wouldn't
compile because methods containing throw are still present in the c++
file.

Sure, writing this without exceptions being thrown was never given any
attention, but there is no reason why we wouldn't accept a patch to change
this.

I had a quick look at directorthrows, and my understanding is that it is
unrelated to my needs. It seems to provide an alternate to director:except
to handle exceptions that happen in python/java. director:except is
already
doing that fine for me. What I need to get rid of is the c++ exceptions
that are being raised when errors happen that are not due to an exception
in the python/java code. Am I understanding that correctly?

Looks like it yes.

I am curious how Java is managing to do directors without c++ exceptions.
How is flow getting from the place where the java code returns to the
place
that was called from java without using exceptions? Do you have an example
of a simple java director use in google3 so that I could look at the
generated code?

It is past my bedtime to look properly right now, but the best example
usage is the java_director_exception_feature_nspace.i testcase. From what I
recall it didn't do the exception handling very well until we changed it to
throw exceptions instead of using JNI to check for pending exceptions. I'll
be intrigued by your patch as the error handling is a lot more challenging
without exceptions.

William

[ojwb]

I just went to remove UnknownExceptionHandler, and realised that it isn't in fact code which has always been dead.

The dh variable removed in 37a08a2 was not in fact unused as the commit message claims - the existence of an object of this type is all that matters - its constructor calls std::set_unexpected() to set the unexpected exception handler, and its destructor restores the old handler.

So I think we should probably revert 37a08a2 and add a testcase to make sure this doesn't get regressed again.

For this issue, a patch to allow SWIG to generate wrappers without using exceptions would not generate that code, or generate it protected by #ifdef SWIG_NO_EXCEPTIONS or similar.

[blaisegassend]

Thanks for digging into that.

I think that code that wants to run without exceptions should be able
define SWIG_DIRECTOR_NO_UEH.

Is there a way to set that flag from the swig file? I expected that putting
it in the begin section would do the trick, but that seems to still put it
around line 3000, which is much too late.

On Thu Jan 29 2015 at 3:58:50 PM Olly Betts [email protected]
wrote:

I just went to remove UnknownExceptionHandler, and realised that it isn't
in fact code which has always been dead.

The dh variable removed in 37a08a2
37a08a2
was not in fact unused as the commit message claims - the existence of an
object of this type is all that matters - its constructor calls
std::set_unexpected() to set the unexpected exception handler, and its
destructor restores the old handler.

So I think we should probably revert 37a08a2
37a08a2
and add a testcase to make sure this doesn't get regressed again.

For this issue, a patch to allow SWIG to generate wrappers without using
exceptions would not generate that code, or generate it protected by #ifdef
SWIG_NO_EXCEPTIONS or similar.

—
Reply to this email directly or view it on GitHub
#315 (comment).

[blaisegassend]

I just updated the patch. This version actually works, but the
UnknownExceptionHandler still needs a way to be disabled since it can't
just be killed. I'm hoping there will be an easy way to define
SWIG_DIRECTOR_NO_UEH.
https://paste.googleplex.com/5576946247270400

On Thu Jan 29 2015 at 3:05:02 PM William S Fulton [email protected]
wrote:

On 29 January 2015 at 20:51, blaisegassend [email protected]
wrote:

Hi William,

Unfortunately, there are four places where director.swig contains
throw/try
statements, which simply cause compilation to fail. So a patch of some
sort
is necessary so that those throws can be eliminated when some feature is
activated. Moreover, director:except only overrides one of the places
where
the director code throws: when the callee has thrown an exception. Other
places that are not covered are: trying to call a pure method, type
mismatch in the value returned by python. So even with director:except
you
can't get rid of all the throwing, and even if it did the code wouldn't
compile because methods containing throw are still present in the c++
file.

Sure, writing this without exceptions being thrown was never given any
attention, but there is no reason why we wouldn't accept a patch to change
this.

I had a quick look at directorthrows, and my understanding is that it is
unrelated to my needs. It seems to provide an alternate to
director:except
to handle exceptions that happen in python/java. director:except is
already
doing that fine for me. What I need to get rid of is the c++ exceptions
that are being raised when errors happen that are not due to an exception
in the python/java code. Am I understanding that correctly?

Looks like it yes.

I am curious how Java is managing to do directors without c++ exceptions.
How is flow getting from the place where the java code returns to the
place
that was called from java without using exceptions? Do you have an
example
of a simple java director use in google3 so that I could look at the
generated code?

It is past my bedtime to look properly right now, but the best example
usage is the java_director_exception_feature_nspace.i testcase. From what I
recall it didn't do the exception handling very well until we changed it to
throw exceptions instead of using JNI to check for pending exceptions. I'll
be intrigued by your patch as the error handling is a lot more challenging
without exceptions.

William

—
Reply to this email directly or view it on GitHub
#315 (comment).

[blaisegassend]

I think that code that wants to run without exceptions should be able
define SWIG_DIRECTOR_NO_UEH.

Is there a way to set that flag from the swig file? I expected that
putting it in the begin section would do the trick, but that seems to still
put it around line 3000, which is much too late.

I was being stupid. This works just fine for disabling UEH (I was writing
section instead of insert):
%insert("begin")
%{
#define SWIG_DIRECTOR_NO_UEH
%}

[blaisegassend]

OK, you can see my current patch here:
blaisegassend/swig@swig:master...python_director_error

I'm not super happy with it because I used %insert("director_error") as the
way to override the exception throwing code which means that if you insert
to that location more than once you'll combine two handlers -- almost
always the wrong thing.

I'm open to suggestions on what the best mechanism to use is as I'm not
very experienced with swig. %pragma seemed like it might be nice except
that it doesn't seem to accept a code block. %feature seemed like it could
mislead you into thinking that you can apply this independently to
different methods/classes, which is not the case. More prosaically, I
wasn't sure what node to use as the first argument to Getattr to get a
global feature setting. I think %feature is probably the better way to go,
but put things this way for now so you have something that actually works
to stare at.

It is past my bedtime to look properly right now, but the best example

usage is the java_director_exception_feature_nspace.i testcase. From what I
recall it didn't do the exception handling very well until we changed it to
throw exceptions instead of using JNI to check for pending exceptions. I'll
be intrigued by your patch as the error handling is a lot more challenging
without exceptions.

I'm not actually trying to solve the hard problem of propagating the
exceptions without using C++ exceptions. I'm just inserting hooks analogous
to director:except so that you can replace C++ exceptions with something
else should you want to. For my use-case, which is testing swigged APIs,
I'm quite happy to terminate the program on exception.

[blaisegassend]

Hi,

Just wanted to see if you'd had a chance to look at the patch. In
particular I'd love to know what my best bet is between %pragma, %feature
and %insert is for activating the feature.

Thanks,
Blaise

On Thu Jan 29 2015 at 10:15:52 PM Blaise Gassend [email protected] wrote:

OK, you can see my current patch here:

blaisegassend/swig@swig:master...python_director_error

I'm not super happy with it because I used %insert("director_error") as
the way to override the exception throwing code which means that if you
insert to that location more than once you'll combine two handlers --
almost always the wrong thing.

I'm open to suggestions on what the best mechanism to use is as I'm not
very experienced with swig. %pragma seemed like it might be nice except
that it doesn't seem to accept a code block. %feature seemed like it could
mislead you into thinking that you can apply this independently to
different methods/classes, which is not the case. More prosaically, I
wasn't sure what node to use as the first argument to Getattr to get a
global feature setting. I think %feature is probably the better way to go,
but put things this way for now so you have something that actually works
to stare at.

It is past my bedtime to look properly right now, but the best example

usage is the java_director_exception_feature_nspace.i testcase. From
what I
recall it didn't do the exception handling very well until we changed it
to
throw exceptions instead of using JNI to check for pending exceptions.
I'll
be intrigued by your patch as the error handling is a lot more challenging
without exceptions.

I'm not actually trying to solve the hard problem of propagating the
exceptions without using C++ exceptions. I'm just inserting hooks analogous
to director:except so that you can replace C++ exceptions with something
else should you want to. For my use-case, which is testing swigged APIs,
I'm quite happy to terminate the program on exception.

[blaisegassend]

Is there any example of a %feature that makes sense only globally?

On Mon Feb 02 2015 at 10:12:24 AM Blaise Gassend [email protected] wrote:

Hi,

Just wanted to see if you'd had a chance to look at the patch. In
particular I'd love to know what my best bet is between %pragma, %feature
and %insert is for activating the feature.

Thanks,
Blaise

On Thu Jan 29 2015 at 10:15:52 PM Blaise Gassend [email protected]
wrote:

OK, you can see my current patch here:
https://github.com/blaisegassend/swig/compare/
swig:master...python_director_error

I'm not super happy with it because I used %insert("director_error") as
the way to override the exception throwing code which means that if you
insert to that location more than once you'll combine two handlers --
almost always the wrong thing.

I'm open to suggestions on what the best mechanism to use is as I'm not
very experienced with swig. %pragma seemed like it might be nice except
that it doesn't seem to accept a code block. %feature seemed like it could
mislead you into thinking that you can apply this independently to
different methods/classes, which is not the case. More prosaically, I
wasn't sure what node to use as the first argument to Getattr to get a
global feature setting. I think %feature is probably the better way to go,
but put things this way for now so you have something that actually works
to stare at.

It is past my bedtime to look properly right now, but the best example

usage is the java_director_exception_feature_nspace.i testcase. From
what I
recall it didn't do the exception handling very well until we changed it
to
throw exceptions instead of using JNI to check for pending exceptions.
I'll
be intrigued by your patch as the error handling is a lot more
challenging
without exceptions.

I'm not actually trying to solve the hard problem of propagating the
exceptions without using C++ exceptions. I'm just inserting hooks analogous
to director:except so that you can replace C++ exceptions with something
else should you want to. For my use-case, which is testing swigged APIs,
I'm quite happy to terminate the program on exception.

[blaisegassend]

Ping?

On Tue Feb 03 2015 at 11:07:12 AM Blaise Gassend [email protected] wrote:

Is there any example of a %feature that makes sense only globally?

On Mon Feb 02 2015 at 10:12:24 AM Blaise Gassend [email protected]
wrote:

Hi,

Just wanted to see if you'd had a chance to look at the patch. In
particular I'd love to know what my best bet is between %pragma, %feature
and %insert is for activating the feature.

Thanks,
Blaise

On Thu Jan 29 2015 at 10:15:52 PM Blaise Gassend [email protected]
wrote:

OK, you can see my current patch here:
https://github.com/blaisegassend/swig/compare/swig:master...
python_director_error

I'm not super happy with it because I used %insert("director_error") as
the way to override the exception throwing code which means that if you
insert to that location more than once you'll combine two handlers --
almost always the wrong thing.

I'm open to suggestions on what the best mechanism to use is as I'm not
very experienced with swig. %pragma seemed like it might be nice except
that it doesn't seem to accept a code block. %feature seemed like it could
mislead you into thinking that you can apply this independently to
different methods/classes, which is not the case. More prosaically, I
wasn't sure what node to use as the first argument to Getattr to get a
global feature setting. I think %feature is probably the better way to go,
but put things this way for now so you have something that actually works
to stare at.

It is past my bedtime to look properly right now, but the best example

usage is the java_director_exception_feature_nspace.i testcase. From
what I
recall it didn't do the exception handling very well until we changed
it to
throw exceptions instead of using JNI to check for pending exceptions.
I'll
be intrigued by your patch as the error handling is a lot more
challenging
without exceptions.

I'm not actually trying to solve the hard problem of propagating the
exceptions without using C++ exceptions. I'm just inserting hooks analogous
to director:except so that you can replace C++ exceptions with something
else should you want to. For my use-case, which is testing swigged APIs,
I'm quite happy to terminate the program on exception.

[blaisegassend]

Not having heard back, I'm wondering if my previous patch was considered
too invasive in some way. So here is an alternate one that does everything
I need using a macro in a similar spirit to SWIG_DIRECTOR_NO_UEH.

https://github.com/swig/swig/pull/349/files

On Mon, Feb 9, 2015 at 11:19 AM Blaise Gassend [email protected] wrote:

Ping?

On Tue Feb 03 2015 at 11:07:12 AM Blaise Gassend [email protected]
wrote:

Is there any example of a %feature that makes sense only globally?

On Mon Feb 02 2015 at 10:12:24 AM Blaise Gassend [email protected]
wrote:

Hi,

Just wanted to see if you'd had a chance to look at the patch. In
particular I'd love to know what my best bet is between %pragma, %feature
and %insert is for activating the feature.

Thanks,
Blaise

On Thu Jan 29 2015 at 10:15:52 PM Blaise Gassend [email protected]
wrote:

OK, you can see my current patch here:
https://github.com/blaisegassend/swig/compare/swig:master...
python_director_error

I'm not super happy with it because I used %insert("director_error") as
the way to override the exception throwing code which means that if you
insert to that location more than once you'll combine two handlers --
almost always the wrong thing.

I'm open to suggestions on what the best mechanism to use is as I'm not
very experienced with swig. %pragma seemed like it might be nice except
that it doesn't seem to accept a code block. %feature seemed like it could
mislead you into thinking that you can apply this independently to
different methods/classes, which is not the case. More prosaically, I
wasn't sure what node to use as the first argument to Getattr to get a
global feature setting. I think %feature is probably the better way to go,
but put things this way for now so you have something that actually works
to stare at.

It is past my bedtime to look properly right now, but the best example

usage is the java_director_exception_feature_nspace.i testcase. From
what I
recall it didn't do the exception handling very well until we changed
it to
throw exceptions instead of using JNI to check for pending exceptions.
I'll
be intrigued by your patch as the error handling is a lot more
challenging
without exceptions.

I'm not actually trying to solve the hard problem of propagating the
exceptions without using C++ exceptions. I'm just inserting hooks analogous
to director:except so that you can replace C++ exceptions with something
else should you want to. For my use-case, which is testing swigged APIs,
I'm quite happy to terminate the program on exception.

[blaisegassend]

Hi William,

No pressure, but I wanted to make sure you're getting the emails in this thread.

I'd like to use directors to make unit-testing the argument parsing in my SWIG APIs easier. Directors would allow me to write those tests without having to write any C++ boiler plate, and only a line or two of Python per method in the API. Unfortunately, Google doesn't allow exceptions to be used, and won take patches to SWIG except if they are also present upstream. So I'm stuck on getting my unit-tests committed.

Thanks,
Blaise

[ojwb]

FWIW, I can't think of any %feature which only makes sense globally, but I may be missing some.

PR #349 looks a plausible approach, though I'd like to see what @wsfulton thinks.

Regarding my note above, which I suspect has been forgotten:

I just went to remove UnknownExceptionHandler, and realised that it isn't in fact code which has always been dead.

The dh variable removed in 37a08a2 was not in fact unused as the commit message claims - the existence of an object of this type is all that matters - its constructor calls std::set_unexpected() to set the unexpected exception handler, and its destructor restores the old handler.

So I think we should probably revert 37a08a2 and add a testcase to make sure this doesn't get regressed again.

If I don't hear anything against doing so, I'll take silence as agreement that 37a08a2 should be reverted.

[blaisegassend]

Given that the UnknownExceptionHandler can be disabled using a #define, it
isn't hindering me at all. In fact, I assumed that it had been reverted
based on the comment you quoted.

On Wed, Mar 11, 2015 at 2:25 PM Olly Betts [email protected] wrote:

FWIW, I can't think of any %feature which only makes sense globally, but
I may be missing some.

PR #349 #349 looks a plausible
approach, though I'd like to see what @wsfulton
https://github.com/wsfulton thinks.

Regarding my note above, which I suspect has been forgotten:

I just went to remove UnknownExceptionHandler, and realised that it isn't
in fact code which has always been dead.

The dh variable removed in 37a08a2
37a08a2
was not in fact unused as the commit message claims - the existence of an
object of this type is all that matters - its constructor calls
std::set_unexpected() to set the unexpected exception handler, and its
destructor restores the old handler.

So I think we should probably revert 37a08a2
37a08a2
and add a testcase to make sure this doesn't get regressed again.

If I don't hear anything against doing so, I'll take silence as agreement
that 37a08a2
37a08a2
should be reverted.

—
Reply to this email directly or view it on GitHub
#315 (comment).

[ojwb]

That part of my comment was aimed at William, since it's his commit I'd be reverting.

[wsfulton]

oops, I've just seen all the comments pinging me, sorry, I've somehow messed up tracking this issue in gmail, so sorry for slowing things down. Anyway...

• Agreed that the director_error was not the correct approach.
• Removing 37a08a2 was a mistake of mine. Mind you no-one has noticed in the 9 odd years since then. Let's not put it back in until I've had a closer look at it as I think the new features and typemaps in the Java directors could be a suitable replacement. I need to remind myself of how it all works again first.
• %feature isn't a global switch as it attaches to one or more C/C++ symbols. C macros are probably the best alternative to a global switch or command line option that we have and can be turned on via 'swig -D' or defining the macro inside %begin.
• I'll get back once I've had a chance to look at the proposed patch in Allow configuration of behaviour when there is a Director Error in Python using define #349. I only really looked at the build system changes so far, which for which I'd like a different implementation.

[ojwb]

Let's not put it back in until I've had a closer look at it as I think the new features and typemaps in the Java directors could be a suitable replacement. I need to remind myself of how it all works again first.

Eh? The removed code was for Python and Ruby, not Java...

But if we don't revert that commit, we should remove UnknownExceptionHandler.

[ojwb]

b655d31 removed UnknownExceptionHandler:

Done in order to be C++17 compliant as it uses std::unexpected_handler
which was removed in C++17. This class was intended for director
exception handling but was never used by SWIG and was never documented.

As discussed above the "never used" isn't really true, but removal seems a reasonable option and deals with part of this issue.

@blaisegassend Did you give up on the rest of this? Realistically it's only likely to get done by someone motivated by an organisational "no exceptions" policy.