[v2,18/24] Catch all std::exception derivatives (including std::bad_alloc, not a std::bad_alloc)

Submitted by Christophe de Dinechin on Feb. 21, 2018, 5:46 p.m.

Details

Message ID 20180221174636.37758-19-christophe@dinechin.org
State New
Headers show
Series "WIP: Refactor the streaming agent towards a more standard C++ style" ( rev: 2 ) in Spice

Not browsing as part of any series.

Commit Message

Christophe de Dinechin Feb. 21, 2018, 5:46 p.m.
From: Christophe de Dinechin <dinechin@redhat.com>

I also added a catch-all clause for double plus extra safety

Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
---
 src/spice-streaming-agent.cpp | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
index f9d8855..26fa910 100644
--- a/src/spice-streaming-agent.cpp
+++ b/src/spice-streaming-agent.cpp
@@ -623,10 +623,14 @@  int main(int argc, char* argv[])
         FrameLog frame_log(log_filename, log_binary);
         agent.CaptureLoop(streamfd, frame_log);
     }
-    catch (std::runtime_error &err) {
+    catch (std::exception &err) {
         syslog(LOG_ERR, "%s\n", err.what());
         ret = EXIT_FAILURE;
     }
+    catch (...) {
+        syslog(LOG_ERR, "Unexpected exception caught (probably thronw by plugin init");
+        ret = EXIT_FAILURE;
+    }
 
     closelog();
     return ret;

Comments

On Wed, 2018-02-21 at 18:46 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin <dinechin@redhat.com>
> 
> I also added a catch-all clause for double plus extra safety
> 
> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> ---
>  src/spice-streaming-agent.cpp | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index f9d8855..26fa910 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -623,10 +623,14 @@ int main(int argc, char* argv[])
>          FrameLog frame_log(log_filename, log_binary);
>          agent.CaptureLoop(streamfd, frame_log);
>      }
> -    catch (std::runtime_error &err) {
> +    catch (std::exception &err) {
>          syslog(LOG_ERR, "%s\n", err.what());
>          ret = EXIT_FAILURE;
>      }
> +    catch (...) {
> +        syslog(LOG_ERR, "Unexpected exception caught (probably thronw by plugin init");

Typo. Is it still technically an exception if it is not derived from
std::exception? I wouldn't call it so, it can be std::string.

I don't like the speculation in the error message. What we should
really do is catch exceptions from plugin init directly and log it
accordingly (and also not necessarily abort the agent, let it run
without the faulty plugin).

Lukas

> +        ret = EXIT_FAILURE;
> +    }
>  
>      closelog();
>      return ret;
> On Feb 22, 2018, at 10:23 AM, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> 
> On Wed, 2018-02-21 at 18:46 +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin <dinechin@redhat.com>
>> 
>> I also added a catch-all clause for double plus extra safety
>> 
>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>> ---
>> src/spice-streaming-agent.cpp | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>> 
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index f9d8855..26fa910 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -623,10 +623,14 @@ int main(int argc, char* argv[])
>>         FrameLog frame_log(log_filename, log_binary);
>>         agent.CaptureLoop(streamfd, frame_log);
>>     }
>> -    catch (std::runtime_error &err) {
>> +    catch (std::exception &err) {
>>         syslog(LOG_ERR, "%s\n", err.what());
>>         ret = EXIT_FAILURE;
>>     }
>> +    catch (...) {
>> +        syslog(LOG_ERR, "Unexpected exception caught (probably thronw by plugin init");
> 
> Typo.

Two, actually…

> Is it still technically an exception if it is not derived from
> std::exception? I wouldn't call it so, it can be std::string.

Well, I think it’s still called an exception even if it’s not std::exception, much like we talk about “functions” or “classes”. What matters is that it is thrown.

> 
> I don't like the speculation in the error message.

Hmmm, you are right. What was I thinking ;-) Clearly, given the two typos and the nonsensical message, I was rushing this a bit too much.


> What we should
> really do is catch exceptions from plugin init directly and log it
> accordingly (and also not necessarily abort the agent, let it run
> without the faulty plugin).

Well, I had a discussion with Frediano on this. He pointed out that things that do not derive from runtime_error are often hard to recover from. So unless we know how to recover, I think it’s a good thing to pop them back at the top and show a message.

We could debate whether a catch-all is a good things.

- Plus side: it lets us send a message to the syslog instead of stderr. Does that matter?
- Minus side: the message is not as clear as, say "terminate called after throwing an instance of ‘int’”, and no abort, so no core dump.

Maybe it’s not a good idea after all…



> 
> Lukas
> 
>> +        ret = EXIT_FAILURE;
>> +    }
>> 
>>     closelog();
>>     return ret;
On Thu, 2018-02-22 at 10:35 +0100, Christophe de Dinechin wrote:
> > On Feb 22, 2018, at 10:23 AM, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> > 
> > On Wed, 2018-02-21 at 18:46 +0100, Christophe de Dinechin wrote:
> > > From: Christophe de Dinechin <dinechin@redhat.com>
> > > 
> > > I also added a catch-all clause for double plus extra safety
> > > 
> > > Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> > > ---
> > > src/spice-streaming-agent.cpp | 6 +++++-
> > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > > index f9d8855..26fa910 100644
> > > --- a/src/spice-streaming-agent.cpp
> > > +++ b/src/spice-streaming-agent.cpp
> > > @@ -623,10 +623,14 @@ int main(int argc, char* argv[])
> > >         FrameLog frame_log(log_filename, log_binary);
> > >         agent.CaptureLoop(streamfd, frame_log);
> > >     }
> > > -    catch (std::runtime_error &err) {
> > > +    catch (std::exception &err) {
> > >         syslog(LOG_ERR, "%s\n", err.what());
> > >         ret = EXIT_FAILURE;
> > >     }
> > > +    catch (...) {
> > > +        syslog(LOG_ERR, "Unexpected exception caught (probably thronw by plugin init");
> > 
> > Typo.
> 
> Two, actually…
> 
> > Is it still technically an exception if it is not derived from
> > std::exception? I wouldn't call it so, it can be std::string.
> 
> Well, I think it’s still called an exception even if it’s not std::exception, much like we talk about “functions” or “classes”. What matters is that it is thrown.
> 
> > 
> > I don't like the speculation in the error message.
> 
> Hmmm, you are right. What was I thinking ;-) Clearly, given the two typos and the nonsensical message, I was rushing this a bit too much.
> 
> 
> > What we should
> > really do is catch exceptions from plugin init directly and log it
> > accordingly (and also not necessarily abort the agent, let it run
> > without the faulty plugin).
> 
> Well, I had a discussion with Frediano on this. He pointed out that things that do not derive from runtime_error are often hard to recover from. So unless we know how to recover, I think it’s a good thing to pop them back at the top and show a message.

I would be interested in the reasoning. Are we still talking about
plugin init? For example if it throws 'int'? I mean it could have done
all sorts of ugly things and no cleanup before throwing the 'int', but
it could have done all those things and _not_ throw even that 'int' too
:) Or do you mean something else?

> We could debate whether a catch-all is a good things.
> 
> - Plus side: it lets us send a message to the syslog instead of stderr. Does that matter?
> - Minus side: the message is not as clear as, say "terminate called after throwing an instance of ‘int’”, and no abort, so no core dump.
> 
> Maybe it’s not a good idea after all…

That sure is a dilemma I don't have a good answer to...

> > 
> > Lukas
> > 
> > > +        ret = EXIT_FAILURE;
> > > +    }
> > > 
> > >     closelog();
> > >     return ret;
> 
>
> On Feb 22, 2018, at 10:51 AM, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> 
> On Thu, 2018-02-22 at 10:35 +0100, Christophe de Dinechin wrote:
>>> On Feb 22, 2018, at 10:23 AM, Lukáš Hrázký <lhrazky@redhat.com> wrote:
>>> 
>>> On Wed, 2018-02-21 at 18:46 +0100, Christophe de Dinechin wrote:
>>>> From: Christophe de Dinechin <dinechin@redhat.com>
>>>> 
>>>> I also added a catch-all clause for double plus extra safety
>>>> 
>>>> Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
>>>> ---
>>>> src/spice-streaming-agent.cpp | 6 +++++-
>>>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>>> 
>>>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>>>> index f9d8855..26fa910 100644
>>>> --- a/src/spice-streaming-agent.cpp
>>>> +++ b/src/spice-streaming-agent.cpp
>>>> @@ -623,10 +623,14 @@ int main(int argc, char* argv[])
>>>>        FrameLog frame_log(log_filename, log_binary);
>>>>        agent.CaptureLoop(streamfd, frame_log);
>>>>    }
>>>> -    catch (std::runtime_error &err) {
>>>> +    catch (std::exception &err) {
>>>>        syslog(LOG_ERR, "%s\n", err.what());
>>>>        ret = EXIT_FAILURE;
>>>>    }
>>>> +    catch (...) {
>>>> +        syslog(LOG_ERR, "Unexpected exception caught (probably thronw by plugin init");
>>> 
>>> Typo.
>> 
>> Two, actually…
>> 
>>> Is it still technically an exception if it is not derived from
>>> std::exception? I wouldn't call it so, it can be std::string.
>> 
>> Well, I think it’s still called an exception even if it’s not std::exception, much like we talk about “functions” or “classes”. What matters is that it is thrown.
>> 
>>> 
>>> I don't like the speculation in the error message.
>> 
>> Hmmm, you are right. What was I thinking ;-) Clearly, given the two typos and the nonsensical message, I was rushing this a bit too much.
>> 
>> 
>>> What we should
>>> really do is catch exceptions from plugin init directly and log it
>>> accordingly (and also not necessarily abort the agent, let it run
>>> without the faulty plugin).
>> 
>> Well, I had a discussion with Frediano on this. He pointed out that things that do not derive from runtime_error are often hard to recover from. So unless we know how to recover, I think it’s a good thing to pop them back at the top and show a message.
> 
> I would be interested in the reasoning. Are we still talking about
> plugin init? For example if it throws 'int'? I mean it could have done
> all sorts of ugly things and no cleanup before throwing the 'int', but
> it could have done all those things and _not_ throw even that 'int' too
> :) Or do you mean something else?

I’ll let Frediano share his point of view. For me, there are two cases:

1) Some std::exception such as bad_alloc. We should be able to deal with them in some reasonable way and recover, but we may not have a very good recovery strategy at the process level, e.g. the system may be low on memory or something like that. So it’s probably better dealt with the program exiting anyway. In any case, we need to catch exception to display the error message, no real point in aborting or core dumping if all that happened was some out-of-memory condition.

2) Some unknown exception, such as 42. More likely an vendor driver exception of some sort. I think it would be good to know what type was thrown and have a core dump. So maybe not catch(…) and let terminate() do its job.

> 
>> We could debate whether a catch-all is a good things.
>> 
>> - Plus side: it lets us send a message to the syslog instead of stderr. Does that matter?
>> - Minus side: the message is not as clear as, say "terminate called after throwing an instance of ‘int’”, and no abort, so no core dump.
>> 
>> Maybe it’s not a good idea after all…
> 
> That sure is a dilemma I don't have a good answer to...
> 
>>> 
>>> Lukas
>>> 
>>>> +        ret = EXIT_FAILURE;
>>>> +    }
>>>> 
>>>>    closelog();
>>>>    return ret;
On Thu, 2018-02-22 at 11:01 +0100, Christophe de Dinechin wrote:
> > On Feb 22, 2018, at 10:51 AM, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> > 
> > On Thu, 2018-02-22 at 10:35 +0100, Christophe de Dinechin wrote:
> > > > On Feb 22, 2018, at 10:23 AM, Lukáš Hrázký <lhrazky@redhat.com> wrote:
> > > > 
> > > > On Wed, 2018-02-21 at 18:46 +0100, Christophe de Dinechin wrote:
> > > > > From: Christophe de Dinechin <dinechin@redhat.com>
> > > > > 
> > > > > I also added a catch-all clause for double plus extra safety
> > > > > 
> > > > > Signed-off-by: Christophe de Dinechin <dinechin@redhat.com>
> > > > > ---
> > > > > src/spice-streaming-agent.cpp | 6 +++++-
> > > > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > > > > index f9d8855..26fa910 100644
> > > > > --- a/src/spice-streaming-agent.cpp
> > > > > +++ b/src/spice-streaming-agent.cpp
> > > > > @@ -623,10 +623,14 @@ int main(int argc, char* argv[])
> > > > >        FrameLog frame_log(log_filename, log_binary);
> > > > >        agent.CaptureLoop(streamfd, frame_log);
> > > > >    }
> > > > > -    catch (std::runtime_error &err) {
> > > > > +    catch (std::exception &err) {
> > > > >        syslog(LOG_ERR, "%s\n", err.what());
> > > > >        ret = EXIT_FAILURE;
> > > > >    }
> > > > > +    catch (...) {
> > > > > +        syslog(LOG_ERR, "Unexpected exception caught (probably thronw by plugin init");
> > > > 
> > > > Typo.
> > > 
> > > Two, actually…
> > > 
> > > > Is it still technically an exception if it is not derived from
> > > > std::exception? I wouldn't call it so, it can be std::string.
> > > 
> > > Well, I think it’s still called an exception even if it’s not std::exception, much like we talk about “functions” or “classes”. What matters is that it is thrown.
> > > 
> > > > 
> > > > I don't like the speculation in the error message.
> > > 
> > > Hmmm, you are right. What was I thinking ;-) Clearly, given the two typos and the nonsensical message, I was rushing this a bit too much.
> > > 
> > > 
> > > > What we should
> > > > really do is catch exceptions from plugin init directly and log it
> > > > accordingly (and also not necessarily abort the agent, let it run
> > > > without the faulty plugin).
> > > 
> > > Well, I had a discussion with Frediano on this. He pointed out that things that do not derive from runtime_error are often hard to recover from. So unless we know how to recover, I think it’s a good thing to pop them back at the top and show a message.
> > 
> > I would be interested in the reasoning. Are we still talking about
> > plugin init? For example if it throws 'int'? I mean it could have done
> > all sorts of ugly things and no cleanup before throwing the 'int', but
> > it could have done all those things and _not_ throw even that 'int' too
> > :) Or do you mean something else?
> 
> I’ll let Frediano share his point of view. For me, there are two cases:
> 
> 1) Some std::exception such as bad_alloc. We should be able to deal with them in some reasonable way and recover, but we may not have a very good recovery strategy at the process level, e.g. the system may be low on memory or something like that. So it’s probably better dealt with the program exiting anyway. In any case, we need to catch exception to display the error message, no real point in aborting or core dumping if all that happened was some out-of-memory condition.

Ok, I mostly agree, possibly with the exception of plugin init. I think
skipping the plugin and keep going might be more robust. Sure, the
plugin will not be unloaded properly, which may cause subsequent errors
elsewhere, but there should be enough information to debug the issue
and the agent is likely to keep working.

> 2) Some unknown exception, such as 42. More likely an vendor driver exception of some sort. I think it would be good to know what type was thrown and have a core dump. So maybe not catch(…) and let terminate() do its job.

I'm inclined to agree too, core dump would certainly be handy for us.

Lukas

> > 
> > > We could debate whether a catch-all is a good things.
> > > 
> > > - Plus side: it lets us send a message to the syslog instead of stderr. Does that matter?
> > > - Minus side: the message is not as clear as, say "terminate called after throwing an instance of ‘int’”, and no abort, so no core dump.
> > > 
> > > Maybe it’s not a good idea after all…
> > 
> > That sure is a dilemma I don't have a good answer to...
> > 
> > > > 
> > > > Lukas
> > > > 
> > > > > +        ret = EXIT_FAILURE;
> > > > > +    }
> > > > > 
> > > > >    closelog();
> > > > >    return ret;
> 
>