Newjavadirexcp #91

Closed
wants to merge 10 commits into
from

2 participants

@marvingreenberg

This is the changes for director exceptions being properly handled, incorporating feedback from W. Fulton. Now this is handled consistently as a feature. If feature support of fragments is added, two helper functions in director.swg can be made into fragments.

marvingreenberg added some commits Jan 30, 2013
@marvingreenberg marvingreenberg Add Feature director:except with documentation and test case
See Doc/Manual/Java.html, 24.5.7 Java Exceptions from Directors,
for details on usage.  Basic functionality allows a director:except
feature to be added to a director method to add a manually written
handler to map java exceptions back to c++ exceptions.  Alternatively,
use new "directorthrows" typemaps to automatically generate handler
blocks for methods in a fashion analogous to the "throws" typemap.

Incorporated changes from discussions with W. Fulton - an implicit
default feature defined in java.cxx can be overridden as documented,
with variable replacements for $directorthrowshandlers and
$defaulthandler.  Default behavior is same as swig 2.0.x (directors
swallow exceptions thrown from java side) but now generate swig
warning 476 for any known exceptions that are not being handled).

Simplified approach for cases with and without exceptions
specifications, see documentation.

Added correct behavior for director exceptions when %catches feature
is used to define exceptions to be handled.
e4a458e
@marvingreenberg marvingreenberg If a pending exception already explains the failure, do not overwrite…
… with a NPE
00333f7
@wsfulton
SWIG member

Just a note that this patch supercedes #20

@wsfulton
SWIG member

@marvingreenberg Can you outline what this patch does that is different to #20 and how it addresses the points brought up on the email thread titled "Java director:except other comments" on swig-devel? Given this was first discussed 8 months ago, I apologise, but I'm finding it difficult to pick up the threads to see what has been addressed and what has not.

@marvingreenberg

@wsfulton You had raised concerns about default handling of exceptions, and had made a variety of suggestions for the general default behavior. I incorporated your suggestion for the message memory holder, and changed the behavior to use the director:except feature consistently instead of sometimes using the feature and sometime using as a tag. The issue about default behavior I handled using the $defaulthandler substitution. This is all changed. I also discovered bugs in the initial patch with regards to supporting %catches and fixed that and added to the test case.

If its convenient, I'd ask you to read the documentation and perhaps look at the one test case, and see if the general approaches are acceptable to you. I agree that following all the discussions from 7 months ago is difficult. The one minor difference between your various suggestions and this implementation was regarding the use of two attributes to handle different default behavior in the presence of exception specications. I don't believe that there is obvious default behavior that makes sense in all situations, and prefer to document the issue and give developers the information they need to understand the issue and make appropriate choices. This is all corner-case, and keeping the basic capapability simpler seems preferable IMO.

@marvingreenberg

The other thing worth mentioning is that all this should be handled very similarly for C#. I'd be happy to look at migrating this over there too.

@wsfulton
SWIG member

I would like the directorthrows typemap to contain all the generated code in the mapping of Java to C++ exceptions. In general I prefer more control given to users, even if it requires more typing (which can be made simpler via macros if need be). So let's change it from your implementation:

%typemap(directorthrows, matches="$packagepath/ExceptionA") MyNS::ExceptionA {
  throw MyNS::ExceptionA(Swig::JavaExceptionMessage(jenv,$error).message());
}

to:

%typemap(directorthrows) MyNS::ExceptionA {
  if (Swig::ExceptionMatches(jenv, $error, "$packagepath/$javaclassname")) {
    throw MyNS::ExceptionA(Swig::JavaExceptionMessage(jenv, $error).message());
  }
}

Note use of common $packagepath and $javaclassname special variables must work. Overall, it allows more control to users, and has the following positives:

  • Two c++ types can now map to one Java exception class via directorthrows typemaps (no hard coded else statements). By this I mean two different directorthrows typemaps can use the same Java exception class and decide to throw a c++ exception depending on say the contents of the exception message. This might be handy given there are no direct equivalents in Java for all the types of c++ exceptions that can be thrown, eg primitive types.
  • Using ExceptionMatches is not obligatory, trust me there will be some unforeseen reason why a user doesn't want to rely on it.
  • An empty typemap will truly do nothing and avoid expensive JNI calls.
  • Once implemented across all the languages supporting directors, this new typemap will have fewer variations and possibly even no variations which is nice.
  • Documenting it and understanding it will be a bit easier to follow.

fullname is not documented, what does it do? Perhaps it is getting mixed up with the %rename fullname attribute, which I can't see being needed.

There are additional brackets in the director:except feature being generated which are not necessary, A user can program:
%feature("director:except") { ... }
instead of
%feature("director:except") %{ ... %}
To add in additional brackets for scoping if desired.

One cosmetic thing, can you get $directordefaulthandlers and $defaulthandler to not generate empty lines if they are not used.

I'm still pondering various aspects of the defaulthandler and will post a few more other comments soon. I'm nearly up to speed again since we last looked at this in depth!

A patch for this in C# will be very welcome. Let's finalize the Java version and get it committed to master first though.

@marvingreenberg
@marvingreenberg

OK, I'm on board with all this.

@wsfulton
SWIG member

Let's get rid of $defaulthandler, it isn't really needed and is an extra complication that doesn't add much. I can't see that providing a different default exception handler for different methods is going to be used very much and so I think the extra complexity is unnecessary given that the same thing can be easily achieved by simply providing a different director:except feature.
Maybe the $defaulthandler is a relic of our discussions on having a different default handler for methods with exception specifications and ones without an exception specification? Let's just make the default handler the same as the other languages, that is throw a DirectorException. So the default will effectively be (coded in java.cxx) and thus look very similar to the other target languages:

%feature("director:except") throwing %{
  jthrowable $error = jenv->ExceptionOccurred();
  if ($error) {
    $directorthrowshandlers
    throw Swig::DirectorException($error);
  }
%}

This will require a DirectorException class based on the ones in Ruby/Python. I'd like a separate exercise at a later point to make these director exception classes consistent between languages as well as adding in the directorthrows typemap support. If you would like to be involved in that after the C# implementation, please shout.

The documentation should cover classes with exception specifications, where the director:except should be written to remove the default handler altogether or replace it with the old equivalent:

jenv->Throw($error);
return $null;

JavaExceptionMessage needs some tweaks:

example_wrap.cxx: In constructor ‘Swig::JavaExceptionMessage::JavaExceptionMessage(JNIEnv*, _jthrowable*)’:
example_wrap.cxx:479: warning: ‘Swig::JavaExceptionMessage::jmsg_’ will be initialized after
example_wrap.cxx:477: warning:   ‘const char* Swig::JavaExceptionMessage::cstr_’
example_wrap.cxx:437: warning:   when initialized here

directors.swg:

    static jmethodID isInstanceMethodID = Class_isInstanceMethod(jenv);

I'm not entirely sure it is safe making isInstanceMethodID static. You might need a call to NewGlobalRef in order to do that, but I would play it safe and just make it non-static. After all, this is exception handling code and would only be called in unusual circumstances, so performance is not deemed critical.
You can probably roll the code in Class_isInstanceMethod into ExceptionMatches - avoid users thinking it can be used, it is internal.
The coding style of additions in directors.swg is quite varied and unlike the rest of the norm/directors.swg.
Try and avoid return statements except at the end of the function too.

@marvingreenberg

+1 on just keeping this simpler. I was trying to split the difference on your earlier proposal.

+1 on always throwing DirectorException on unhandled, by default. It is worth mentioning this will be a non-backwards compatible change since existing code just silently swallows these exceptions. But this is swig 3.0, so a reasonable time to improve the default behavior.

Re: MethodId. Method IDs are just integer indexes on a class. So no issue of global reference or lifecycle. But, fine.

marvingreenberg added some commits Oct 4, 2013
@marvingreenberg marvingreenberg Remove unneeded fullname attributes 9615cf6
@marvingreenberg marvingreenberg director.swg
Fix initializer order
Fix formatting and remove interior returns
Removed static use of methodID index
8eda064
@marvingreenberg

One more note about this. When examining the code more closely in the existing python director exception to add the generic DirectorException, I realized I can do something equivalent for java, where the thrown java exception can be reconstructed generically when returning to the Java layer if there is no need for mapping to specific C++ exceptions. I'm including that with an additional test.

I also noticed that python and ruby director.swg provide an unexpected exception handler that warns at runtime when an unallowed exception is thrown from a director method (allows suppression with SWIG_DIRECTOR_NO_UEH). This may make sense for Java and other languages for the issue of exception specifications, especially in light of keeping cross language behavior the same. Although looking a little more now, it is unclear this is actually used - I can not find any reference in doc or generated code to when this handler would be installed.

@wsfulton
SWIG member
@marvingreenberg

I was planning on skipping the UEH bit, just mentioned it. Probably should have been over on swig-devel.

Just a bit of status - I was DONE, just doing some testing for the director exception stuff the way use it on our project, and found another case that was broken -- when you have %nspace without -package. The code in java.cxx that resolves types has lots of little patches to handle how the different $packagepath $javaclassname (sic) strings get substituted. I went down quite a few rabbit holes and am trying to unwind all that.

marvingreenberg added some commits Oct 15, 2013
@marvingreenberg marvingreenberg director.swg:
Add DirectorException based on python DirectorException, holding information
to allow reconstruction of java-exception cause.

Restructure ExceptionMessage holder class, adding JavaString used by
ExceptionMessage and DirectorException.

java.cxx:

Refactor substituteClassname, substitutePackagePath to properly handle
different permutations of %nspace and -package when substituting
$packagepath and $packagepath/$javaclassname combinations in typemaps
for director exceptions.

Get rid od $defaulthandler and change default to always throw a
Swig::DirectorException when an unmapped java exception occurs in a
director method.

Get rid of extra newline when no directorthrows handlers, extra brackets.

Other:
Add/update tests and update documentation.
329282c
@marvingreenberg marvingreenberg Fixes after reviewing doc changes. ae1abc8
@marvingreenberg

@wsfulton I think this addresses your comments and the issue I discovered with %nspace without a -package.

@wsfulton
SWIG member

Okay, I think we are just about there.

Have you looked at the total diffs in java.cxx that you have added? There are a number of whitespace changes. I don't see the point of these, but can deal with this but creates work. Why introduce substituteClassnameAndPackagePath which does the same thing as substituteClassname?

In 329282c, you have:

Refactor substituteClassname, substitutePackagePath to properly handle
different permutations of %nspace and -package when substituting
$packagepath and $packagepath/$javaclassname combinations in typemaps
for director exceptions.

I'd like to understand the motivation for this (outside of director exceptions). If it is just for director exceptions, I don't understand why they are different to elsewhere... can you give a motivating example, is part of one of the .i added files relevant?

I see a number of TODO in the patch, I like completed patches!

@marvingreenberg

Re: extra method. Its hard to follow the language module code - the extra method name makes the intent clearer at the call. But, whatever you say, I'll type. If you want "substituteClassname" to also substitute the $packagepath depending on the arguments, I'll just get rid of the other method.

The short answer for why refactor is to fix a test failure. The replacement of $javaclassname happens several different places, inconsistently. My requirement was to replace $javaclassname with a descriptor-style full path. This is what canonicalizeJNIdescriptor does. But that could not be used because the string passed for substitution for director exceptions is an entire code block. A "." may occur in various places besides the descriptor, so simply globally replacing "." with "/' does not work. Moving this code up into substituteClassnameAndPackagePath allows the substitution to be done with needed context.

Personally, I think the correct solution is to have $packagepath always be the path form of whatever is passed to -package (with trailing "/" magic for empty) and have $javaclassname and $javadescriptor substitute consistently and always be FULLY qualified (including package) , regardless of which combination of "-package", javapackage typemap, and %nspace. This is better than complex magic about $javaclassname getting substituted many different ways. But that would be a fairly extensive and incompatible change.

@marvingreenberg

@wsflton BTW, I merged this onto latest master locally -- the only conflicts are an issue with the one added warning in the first commit.

@wsfulton
SWIG member

FYI, I'm in the middle of merging and editing this request, so expect to see it committed in a day or two when I get another free moment.

@marvingreenberg

Great, thanks. I've started working the task to do this for c#. The exception handling and director implementation in c# is actually quite a bit different, so I suspect there will need to be some discussion. But I'm mostly just getting my c# "sea-legs" right now.

@wsfulton
SWIG member

Great, that sounds good. There are a number of edits I've made to your patches and I'd prefer it if you used the modified versions as a base for C#, so just getting your 'C# legs' is probably the right approach for now!

@marvingreenberg

I suspect all your patches are related to a different way to substitute $packagepath and $classname. Those changes are irrelevant for csharp since there is no $packagepath variable and the substitutions are all just substitutions, no magic.

@wsfulton
SWIG member

I'm please to say that I've finally merged this into master. Thanks for your work, it is a useful enhancement. I combined your commits into one committed patch, as it had quite a bit of noise in its development/history. I've since edited it quite heavily with comments as to what/why. Hopefully you have the time to go through those as a useful learning experience for SWIG development. Thanks again. William

@wsfulton wsfulton closed this in 6736e74 Nov 7, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment