Major revision for error handling in the interpreter

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Major revision for error handling in the interpreter

John W. Eaton
Administrator
With the changeset

   http://hg.savannah.gnu.org/hgweb/octave/rev/fd32c1a9b1bd

the "error" function (and similar functions that ultimately call it),
simply throws an exception that contains information about the error
(message, id, stack info) instead of printing an error message
immediately and then throwing an exception.

It is now the responsibility of any code that catches an execution
exception to determine whether and when to display error messages.  I
believe that I have made all the necessary changes so that errors are
still displayed where they should be, but I may have missed something.
If you notice that error messages are not displayed correctly, please
submit a bug report for the problem.

The new approach is more flexible and allows for some simplification of
the error message routines as they no longer need feedback from the
interpreter to know when to display or buffer messages.

jwe

Reply | Threaded
Open this post in threaded view
|

Re: Major revision for error handling in the interpreter

Pantxo
John W. Eaton wrote

> With the changeset
>
>    http://hg.savannah.gnu.org/hgweb/octave/rev/fd32c1a9b1bd
>
> the "error" function (and similar functions that ultimately call it),
> simply throws an exception that contains information about the error
> (message, id, stack info) instead of printing an error message
> immediately and then throwing an exception.
>
> It is now the responsibility of any code that catches an execution
> exception to determine whether and when to display error messages.  I
> believe that I have made all the necessary changes so that errors are
> still displayed where they should be, but I may have missed something.
> If you notice that error messages are not displayed correctly, please
> submit a bug report for the problem.
>
> The new approach is more flexible and allows for some simplification of
> the error message routines as they no longer need feedback from the
> interpreter to know when to display or buffer messages.
>
> jwe

I have tried to call "error" from the GUI thread, namely in
gl2ps_renderer::draw_axes, and this now crashes Octave. Does this mean that
we need to redo the mechanism for rethrowing exceptions in
octave_qapplication::notify and GLCanvas::do_print?

Pantxo



--
Sent from: https://octave.1599824.n4.nabble.com/Octave-Maintainers-f1638794.html

Reply | Threaded
Open this post in threaded view
|

Re: Major revision for error handling in the interpreter

John W. Eaton
Administrator
On 10/6/19 5:39 PM, Pantxo wrote:

> I have tried to call "error" from the GUI thread, namely in
> gl2ps_renderer::draw_axes, and this now crashes Octave. Does this mean that
> we need to redo the mechanism for rethrowing exceptions in
> octave_qapplication::notify and GLCanvas::do_print?

No, we should not need to change that.  Can you give me a recipe for
reproducing the crash?

Previously, the error function would print a message, then throw an
exception.  There were ways to save or discard the message for later
handling, but that was not the default.  Now the message is stored as
part of the exception object, then we throw the exception and always
have to handle displaying the message when the exception is caught.

But we are still throwing an execution_exception object, so catching
that in the GUI thread and calling the event_manager to queue an error
in the interpreter thread should still work as before.  If it is failing
now, then it was not an intentional change.

jwe


Reply | Threaded
Open this post in threaded view
|

Re: Major revision for error handling in the interpreter

Pantxo
John W. Eaton wrote
> On 10/6/19 5:39 PM, Pantxo wrote:
>
> No, we should not need to change that.  Can you give me a recipe for
> reproducing the crash?

Here is the intensional error I introduced for testing purpose:

diff -r 9e526393d80a libinterp/corefcn/gl2ps-print.cc
--- a/libinterp/corefcn/gl2ps-print.cc Sun Oct 06 22:25:36 2019 +0200
+++ b/libinterp/corefcn/gl2ps-print.cc Mon Oct 07 10:09:18 2019 +0200
@@ -131,6 +131,7 @@
 
     void draw_axes (const axes::properties& props)
     {
+      error ("error encountered in the GUI thread");
       // Initialize a sorting tree (viewport) in gl2ps for each axes
       GLint vp[4];
       m_glfcns.glGetIntegerv (GL_VIEWPORT, vp);

Then in Octave:

octave:1> plot (1:10); print /tmp/toto.eps
Input file "/tmp/oct-g6hKlo.eps" is not EPSF.
octave:2> fatal: caught signal Erreur de segmentation -- stopping myself...
Erreur de segmentation

Pantxo




--
Sent from: https://octave.1599824.n4.nabble.com/Octave-Maintainers-f1638794.html

Reply | Threaded
Open this post in threaded view
|

Re: Major revision for error handling in the interpreter

Pantxo
Pantxo wrote
> octave:1> plot (1:10); print /tmp/toto.eps
> Input file "/tmp/oct-g6hKlo.eps" is not EPSF.
> octave:2> fatal: caught signal Erreur de segmentation -- stopping
> myself...
> Erreur de segmentation
>
> Sent from:
> https://octave.1599824.n4.nabble.com/Octave-Maintainers-f1638794.html

Then if I call "error" again in the interpreter thread (instead of
rethrowing the exception), I can prevent the crash, but the error message is
lost and the error is interpreted as a parse error (as it used to be):

diff -r 9e526393d80a libgui/graphics/GLCanvas.cc
--- a/libgui/graphics/GLCanvas.cc Sun Oct 06 22:25:36 2019 +0200
+++ b/libgui/graphics/GLCanvas.cc Mon Oct 07 13:06:35 2019 +0200
@@ -183,11 +183,11 @@
         catch (octave::execution_exception& e)
           {
             emit interpreter_event
-              ([] (void)
+              ([e] (void)
                {
                  // INTERPRETER THREAD
 
-                 std::rethrow_exception (std::current_exception ());
+                 error ("%s", e.message ().c_str ());
                });
           }

With this change I obtain:

octave:1> plot (1:10); print /tmp/toto.eps
Input file "/tmp/oct-X5l7zO.eps" is not EPSF.
octave:2> error: parse error
octave:2>

So I guess there is something better we can do so that at least the original
error message isn't lost, and we ideally avoid this erroneous "parse error"
message.
 
Pantxo



--
Sent from: https://octave.1599824.n4.nabble.com/Octave-Maintainers-f1638794.html

Reply | Threaded
Open this post in threaded view
|

Re: Major revision for error handling in the interpreter

John W. Eaton
Administrator
On 10/7/19 7:11 AM, Pantxo wrote:

> Pantxo wrote
>> octave:1> plot (1:10); print /tmp/toto.eps
>> Input file "/tmp/oct-g6hKlo.eps" is not EPSF.
>> octave:2> fatal: caught signal Erreur de segmentation -- stopping
>> myself...
>> Erreur de segmentation
>>
>> Sent from:
>> https://octave.1599824.n4.nabble.com/Octave-Maintainers-f1638794.html
>
> Then if I call "error" again in the interpreter thread (instead of
> rethrowing the exception), I can prevent the crash, but the error message is
> lost and the error is interpreted as a parse error (as it used to be):
>
> diff -r 9e526393d80a libgui/graphics/GLCanvas.cc
> --- a/libgui/graphics/GLCanvas.cc Sun Oct 06 22:25:36 2019 +0200
> +++ b/libgui/graphics/GLCanvas.cc Mon Oct 07 13:06:35 2019 +0200
> @@ -183,11 +183,11 @@
>           catch (octave::execution_exception& e)
>             {
>               emit interpreter_event
> -              ([] (void)
> +              ([e] (void)
>                  {
>                    // INTERPRETER THREAD
>  
> -                 std::rethrow_exception (std::current_exception ());
> +                 error ("%s", e.message ().c_str ());
>                  });
>             }
>
> With this change I obtain:
>
> octave:1> plot (1:10); print /tmp/toto.eps
> Input file "/tmp/oct-X5l7zO.eps" is not EPSF.
> octave:2> error: parse error
> octave:2>
>
> So I guess there is something better we can do so that at least the original
> error message isn't lost, and we ideally avoid this erroneous "parse error"
> message.

I pushed the following changeset:

   http://hg.savannah.gnu.org/hgweb/octave/rev/458adc344819

It seems to avoid the crash for me (thanks for the tip) and doesn't
change errors in the event manager callback functions to be labeled as
parse errors.

jwe

Reply | Threaded
Open this post in threaded view
|

Re: Major revision for error handling in the interpreter

Pantxo
Le 07/10/2019 à 21:19, John W. Eaton a écrit :

> On 10/7/19 7:11 AM, Pantxo wrote:
>> Pantxo wrote
>>> octave:1> plot (1:10); print /tmp/toto.eps
>>> Input file "/tmp/oct-g6hKlo.eps" is not EPSF.
>>> octave:2> fatal: caught signal Erreur de segmentation -- stopping
>>> myself...
>>> Erreur de segmentation
>>>
>>> Sent from:
>>> https://octave.1599824.n4.nabble.com/Octave-Maintainers-f1638794.html
>>
>> Then if I call "error" again in the interpreter thread (instead of
>> rethrowing the exception), I can prevent the crash, but the error
>> message is
>> lost and the error is interpreted as a parse error (as it used to be):
>>
>> diff -r 9e526393d80a libgui/graphics/GLCanvas.cc
>> --- a/libgui/graphics/GLCanvas.cc    Sun Oct 06 22:25:36 2019 +0200
>> +++ b/libgui/graphics/GLCanvas.cc    Mon Oct 07 13:06:35 2019 +0200
>> @@ -183,11 +183,11 @@
>>           catch (octave::execution_exception& e)
>>             {
>>               emit interpreter_event
>> -              ([] (void)
>> +              ([e] (void)
>>                  {
>>                    // INTERPRETER THREAD
>>   -                 std::rethrow_exception (std::current_exception ());
>> +                 error ("%s", e.message ().c_str ());
>>                  });
>>             }
>>
>> With this change I obtain:
>>
>> octave:1> plot (1:10); print /tmp/toto.eps
>> Input file "/tmp/oct-X5l7zO.eps" is not EPSF.
>> octave:2> error: parse error
>> octave:2>
>>
>> So I guess there is something better we can do so that at least the
>> original
>> error message isn't lost, and we ideally avoid this erroneous "parse
>> error"
>> message.
>
> I pushed the following changeset:
>
>   http://hg.savannah.gnu.org/hgweb/octave/rev/458adc344819
>
> It seems to avoid the crash for me (thanks for the tip) and doesn't
> change errors in the event manager callback functions to be labeled as
> parse errors.
>
> jwe

Thanks, that fixes both the crash and the error message:

octave:1> plot (1:10); print /tmp/toto.eps
Input file "/tmp/oct-DpcUlf.eps" is not EPSF.
octave:2> error: error encountered in the GUI thread
error: called from
     __opengl_print__ at line 196 column 7
     print at line 749 column 16
octave:2>


Pantxo