CH

Christian Hoff

Tue, Oct 16, 2018 8:46 PM

I am fordwarding this mail to the mailing list as well.

On 16/10/18 22:42, Christian Hoff wrote:

Hello Ming,

you are absolutely right and I am sorry for the confusion. Of course
the SWIG Java bindings throw "java.lang.Exception" and not the
"RuntimeException" as I assumed.

You actually discovered a bug in my patch. The problem with my patch
is that I am throwing a java.lang.Exception from the Java JNI code,
but this Exception is now no longer mentioned in the "throws"
specification of the Java methods generated by SWIG - even though it
is a checked exception. In fact my change (unintentionally) removed
the "throw" specification also on the Java side! For this reason you
no longer get compile errors when omitting the "catch" blocks in your
sample code. This is a bug. Because the Exception is thrown from JNI
C++ code, Java cannot find out that the generated C++ JNI code is
actually violating the Java exception specification of the generated
method (which declares that no checked exceptions should be thrown).

After some reading of the SWIG documentation, I found out that there
is a relatively simple way to fix this issue and to restore the
"throw" specification on the Java side. To do this, please replace the
line no. 50 in file "pjsip-apps/src/swig/pj_error.i" by

%javaexception("java.lang.Exception") {

This should restore the "throw" specification for all PJSIP Java
methods. I tested this briefly and it works. One possible downside of
this change is however that this change will add a "throw Exception"
specification to all generated Java methods inside PJSIP, so every
PJSIP Java generated method now explicitly throws java.lang.Exception.
This could be an issue if there are some PJSIP methods that cannot
throw "pj::Error". In this case the user has to handle an exception
that can never occur. However I am not even sure if such methods exist.

Another option would be throw non-checked exceptions - such as
RuntimeException - from the PJSIP Java bindings. But I think you like
the idea of exception specifications and checked exceptions and
therefore probably the first option will be the better one. As I said,
the fix works by just changing a single line.

Best regards,

Christian

On 16/10/18 11:24, Ming wrote:

Hi Christian,

Thanks for your detailed explanation, which I'm sure help us
(especially me) improve our knowledge about Java.

After checking the code (in pjsua2.i), our current exception actually
uses java.lang.Exception, which, according to the reference below,
belongs to checked exceptions. I verified this by trying to compile
our sample code and remove a try-catch block and it did fail to
compile.

Ref:https://www.geeksforgeeks.org/checked-vs-unchecked-exceptions-in-java/

Best regards,
Ming

On Tue, Oct 16, 2018 at 2:45 PM, Christian Hoffchristian_hoff@gmx.net wrote:

Hello Ming,

thank you very much for your quick feedback and I am happy to hear that the
propagation of exceptions works well with my patch!

Regarding your point about exceptions and thr "throw (...)" specification:
You are right in saying that an exception specification is still an
important in programming languages such as Java. However in Java two kinds
of exceptions exist and only one kind of them (the checked exception) works
with exception specifications:

Checked exceptions. Checked exceptions are the kind of exceptions that you
are talking about, where the programmer is forced to somehow handle the
exception. Handling the exception can mean either installing a catch handler
or adding this exception type to the "throws" declaration of your method. If
the exception is not handled, a compile error will be generated - as you
wrote.
Unchecked exceptions. Unchecked exceptions are the kind of exceptions that
the programmer is not forced to handle. For this kind of exception, the
programmer is not forced to install a "catch" handler, so no compile error
will be generated if the exception is not handled at all. Such exceptions
also do not appear in the "throws" declaration of the methods. Unchecked
exceptions are all exceptions that derive from the RuntimeException and
Error classes.

For PJSIP a "pj::Error" was mapped to a RuntimeException up to now (already
before my changes), so a "pj::Error" from the C++ PJSIP library raises an
unchecked exception. This is true before and after my changes. This means
that - as of now - a Java programmer is not forced to handle a "pj::Error"
from the C++ code. This is because the "pj::Error" will be mapped to an
unchecked exception. The consequence is that there is no compile error if
the user forgets to install a catch handler on the Java side for errors from
PJSIP. The exception for the "pj::Error" also does not appear in the
"throws" declaration of the Java methods generated by SWIG as only checked
exceptions should appear there, but not unchecked exceptions such as
"RuntimeException".

What I want to say in short is that my patch does not change anything in
terms of exception handling on the Java side. The old behaviour was already
to throw an unchecked exception that the user was not required to catch and
the new approach is the same. And already before my changes we were not
using exception specifications on the Java side as we were using unchecked
exceptions. Or have I missed or overlooked something?

What we might want to consider for the PJSIP Java bindings is to change the
exception handling there to throw checked exceptions if a "pj::Error" is
thrown from the C++ code, in order to force users to handle a "pj::Error".
But this is more or less a separate change that does not really have
anything to do with my changes.

Thanks again for the quick testing and the quick feedback!

Have a nice day and best regards,

  Christian

On 16/10/18 06:31, Ming wrote:

Hi Christian,

I did some testing here and the exception does work great during runtime
now!

However, in my opinion, the "throw()" specification is still important
for SWIG. While it's true that throw() is deprecated for C++, it
remains a valid (and important) part of other languages such as Java.
And having throw() in the declaration of the function will help the
programmers during the development process.

For example, if you try to call a function which throws an exception
in Java but forget to put try and catch, the compiler will give an
error such as:
sample.java:155: error: unreported exception Exception; must be caught
or declared to be thrown

But it's now gone, so the developer must now carefully check each
function in order to know whether it can throw an exception.

Best regards,
Ming

On Mon, Oct 15, 2018 at 10:18 AM, Mingming@teluu.com wrote:

Hi Christian,

Thank you for the hard work. Let us review and test this. Given the
scale of the platforms and language binding affected, this may take
some time. We will update you again.

Best regards,
Ming

On Mon, Oct 15, 2018 at 4:19 AM, Christian Hoffchristian_hoff@gmx.net
wrote:

Hello all,

during the last weeks, I worked hard on the patch for PJSUA2 and C++ 17
compatibility again. For those who have forgotten: Currently the PJSUA2
headers cannot be compiled with C++ 17 because C++ 17 removes support for
exception specifications. But unfortunately PJSUA2 makes have use of
exception specifications.

The open issue with my last patch was that SWIG no longer generates catch
handlers that catch "pj::Error", as Ming pointed out. I have now updated the
patch so that SWIG also generates the required catch handlers. Doing so has
been a quite difficult, tedious and time-consuming task. But finally I am
done and I am happy to share the resulting patch for inclusion into pjsip.
The updated patch is attached to this mail.

The magic on the SWIG side is done with the "%exception" directive, which
installs a default global exception handler. In my updated patch, I am now
using this "%exception" directive to generate the catch handler for
"pj::Error" (see the new file "pjsip-apps/src/swig/pj_error.i"). The
rationale behind this was that I really wanted to remove all the "throw
(pj::Error)" from the header files, so I did not want to have a "PJ_THROW"
that would expand to nothing during the compile and expand to the old "throw
(...)" when using SWIG. "throw (...)" is now deprecated syntax and we should
not keep it around for the sake of SWIG in my opinion.

Some basic testing was also done:

To verify the C++ 17 compatibility, I compiled my application with the
installed adopted pjsua2 header files.
To verify the generated SWIG language bindings and catch handlers:

For the Python language binding, I verified that the compile works.
Afterwards I installed the generated language binding and ran the test file
"test.py". "test.py" also verifies the exception handling. I also inspected
the generated catch handlers for pj::Error visually.
For the Java and C# bindings, I verified that the compile works and
inspected the generated catch handlers visually. Unfortunately I did not
find a suitable unit test for the exception handling, but I am optimistic
that it works nevertheless. I could not run the generated code for the C#
bindings because I do not have Xamarin installed on my machine. But as I
said, at least I can compile the C++ wrapper code for C#.

I hope that this patch can now be considered for inclusion into PJSIP.

Best regards,

  Christian

On 25/09/18 10:26, Max Truxa wrote:

Hello Christian,

how about replacing the throw specifications with a macro that expands
to nothing when compiling as C++ but throw(Error) when being parsed
by SWIG?

Something like this:

#ifdef SWIG // defined if SWIG is parsing the file

define PJ_CAN_THROW throw(Error)

#else

define PJ_CAN_THROW

#endif

struct Foo
{
void bar() PJ_CAN_THROW;
};

Regards,

Max

On Tue, Sep 25, 2018 at 8:30 AM Christian Hoffchristian_hoff@gmx.net
wrote:

Hello Ming,

this is also an interesting idea. But will this actually work..? I am
wondering because SWIG only parses the header files of PJSUA2. However
the actual function implementation is in the ".cpp" source files and in
these files you can find the code that throws the exceptions and that
could use "PJ_THROW()". But to my knowledge SWIG does not parse the
source files and would thus not know which functions may call
"PJ_THROW". Or am I wrong about this?

Best regards,

  Christian

On 25/09/18 03:08, Ming wrote:

Hi Christian,

We haven't found a good way either. Our idea is to put PJ_THROW()
macro to replace throw(), which can somehow be used by SWIG to
generate the exception specs.

Regards,
Ming
On Tue, Sep 25, 2018 at 4:21 AM Christian Hoffchristian_hoff@gmx.net
wrote:

Hello Ming and all,

I have now done some research into SWIG and exceptions. After reading
the SWIG documentation, I found out that SWIG indeed parses the C++
exception specification to figure out for what types of exceptions
"catch(..)" should be generated (see
http://www.swig.org/Doc3.0/SWIGDocumentation.html#SWIGPlus_exception_specifications
). Exception types that are specified in the exception specification are
automatically propagated to the higher language (Java, C# etc.). Now
that we need to remove the exception specification, we lose that
capability. Thanks to Ming for the hint. However there is the
possibility to adapt the SWIG interface file
("pjsip-apps/src/swig/pjsua2.i") and to add explicit "%catches"
statements for each method that describe which types of exceptions the
method could possibility throw. So I guess I need to rework my patch for
this. I just hope that I will find some way to avoid having to annotate
every method with "%catches".... I'll see. Or if someone knows how to do
it, then I would be very grateful for a reply.

Best regards,

    Christian

On 24/09/18 12:43, Christian Hoff wrote:

Hello Ming,

thanks for your reply! I will have a look into the SWIG topic this
evening and will let you know my findings.

Best regards,

  Christian

On 24/09/18 09:03, Ming wrote:

Hi Christian,

We apologize for the delay in response. We're currently reviewing it
and have also managed to do some testing. While for C/C++, this seems
to be okay, we're still considering how to best handle this for our
Android and Java (via SWIG binding), which may still need the "throw"
specifications in order for the developers to know that the functions
may throw some exceptions.

If you have any feedback regarding this, please let us know.

Best regards,
Ming

On Mon, Sep 24, 2018 at 2:32 PM Christian Hoff
christian_hoff@gmx.net wrote:

Hello all,

some feedback for this patch is still appreciated. In general I am
getting somewhat frustrated about the collaboration with people who
want
to contribute to the project. Already in the past other people have
posted patches here to this list, but did not get any response - not
even a response what needs to be changed for the patch to be
accepted. I
think this is not a good mode of collaboration. Everyone who posts a
patch here to this mailing list has done this with the intention to
help
the project PJSIP to move along (and has spent much time developing the
patch), so you should at least return the favour and review the patch.

This particular issue that I am trying to address with my patch (that
the PJSUA2 headers don't compile against C++17) is an issue that you
will eventually need to fix anyhow, so why not fix it right now? As of
now, modern versions of GCC use C++ 14 by default, it is only a matter
of time until C++ 17 will become the default and people will start
complaining that there are compile errors from the PJSUA2 headers.

Besides this, my own app uses PJSUA2 with C++ 17 features, so I would
have to revert back to C++14 if this patch is not accepted upstream.
This would be really unnecessary. So I would really appreciate a patch
review.

Best regards,

   Christian

On 18/09/18 20:57, Christian Hoff wrote:

Hello all,
Hello Ming & Riza,

do you have any feedback regarding this patch for C++ 17 support? I am
also willing to rework it if required, but some feedback would be
nice.

Thanks in advance and kind regards,

    Christian

On 12/09/18 18:23, Christian Hoff wrote:

Hello all,

currently it is not possible to compile a pjsua2 application with
C++17. The reason is that C++17 removes support for dynamic exception
specifications (that is "throws ..." declarations at the end of a
function). Unfortunately PJSUA2 makes heavy use of exception
specifications and most PJSUA2 functions are declared as "throw
(Error)" - meaning that they can throw an exception of type "Error"
that the programmer is supposed to catch.

The consequence is that a lot of compile errors can be observed when
compiling a pjsua2 application with C++17. The compile errors look as
follows:

/usr/local/include/pjsua2/json.hpp:80:33: error: ISO C++17 does not
allow dynamic exception specifications
virtual string saveString() throw(Error);
^~~~~

The compile of the same application worked fine when using the C++14
standard, but it got broken when I switched from C++14 to the C++17
standard.

See
https://www.bfilipek.com/2017/05/cpp17-details-fixes-deprecation.html#removing-deprecated-exception-specifications-from-c17

and
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/p0003r0.html
for more background about the removal of exception specifications.

To make the application compile again, I had to remove "throw(Error)"
in the whole PJSUA2 source code and include files. This seems to be
the only viable and good fix for this issue. I wrote my own "sed"
script to do that and made some cosmetic corrections afterwards. Then
I generated the attached patch file. Could you please apply this
patch to the code base or give me feedback what I still need to
improve? I verified that pjsip compiles fine with the patch applied.

Thank you very much!

Best regards,

   Christian