Re: [Spice-devel] [PATCH v2 22/24] Throw exception in case of write failure

[Lukáš Hrázký]

On Thu, 2018-02-22 at 15:09 +0100, Christophe de Dinechin wrote:
> > On 22 Feb 2018, at 14:12, Lukáš Hrázký  wrote:
> > 
> > Apart from what's below, I'm also thinking if it wouldn't be better to
> > leave this addition of exception classes for later and concentrate on
> > finishing what's already present in this big series. Adding more stuff
> > to it doesn't really speed things up :/
> > 
> > I think write_all() throwing a runtime_error as of now is surely no biggie.
> 
> It is a biggie. I hoped that your interest in writing unit tests would make 
> it obvious.

Note I wrote "as of now", meaning the exceptions could be pimped up
later and not grow this series. Just a suggestion anyway.

> It’s big enough to be E13 in the C++ Core Guidelines, 
> https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Re-exception-types
> 
> "The standard-library classes derived from exception should be used only as 
> base classes or for exceptions that require only "generic" handling. Like 
> built-in types, their use could clash with other people's use of them."
> 
> Let me stand my ground here. I will not throw runtime_error in application 
> code I write. I personally consider this “lazy”. I strongly invite you to not 
> do it for the reasons already exposed, and if you don’t, I will propose 
> patches to fix what I see as a design bug.

No problem. I agree it is lazy, actually. You have my permission to
call me that. :P

> 
> > On Thu, 2018-02-22 at 12:27 +0100, Christophe de Dinechin wrote:
> > > > On 22 Feb 2018, at 10:42, Lukáš Hrázký  wrote:
> > > > 
> > > > On Wed, 2018-02-21 at 18:46 +0100, Christophe de Dinechin wrote:
> > > > > From: Christophe de Dinechin 
> > > > > 
> > > > > This also introduces the spice::streaming_error::Error class, which 
> > > > > we can reuse
> > > > > later as a base class for all agent-specific errors. This class 
> > > > > provides a
> > > > > formatted 'message()' class that returns a string, making it easier 
> > > > > to format
> > > > > errors without allocating memory at throw-time.
> > > > 
> > > > I am not at all convinced this approach has any advantages.
> > > 
> > > I realize that I only shared the benefits in a private message, not 
> > > on-list. So here:
> > > 
> > > The benefit I see in this approach are
> > > 
> > > 1. there is no alloc at the throw point,
> > 
> > I don't see this as a benefit in itself, are there any implications I’m 
> > missing?
> 
> At the throw point, the maximum amount of resources is allocated, and 
> throwing the wrong exception will do the maximum amount of damage In 
> particular, if you want to retry on a write error or do some specific cleanup 
> and rethrow, getting a bad_alloc will kill these possibilities.
> 
> At the catch point, you have already discriminated what you could deal with, 
> you were able to securely do retries or anything you wanted to do with 
> exceptions you caught, and if after everything else fails, you get an error 
> while printing the error message, that’s the minimum amount of possible 
> damage.
> 
> But as I already wrote, if that’s so much of a concern, we can rewrite the 
> message-generation code so that it never allocates.
> 
> > 
> > > 2. the catch point can extract a specific exception type and its args 
> > > (e.g. for unit testing)
> > 
> > This is true, but as I said, I consider this useful only for specific 
> > cases. Often you don't care that much to differentiante.
> 
> This “you” is not me. I certainly do care that I can discriminate my 
> exceptions.
> 
> > In unit tests, you can also check the error message.
> 
> You could, does not make it a good idea. IMO, it’s about as bad as
> 
>   throw “That did not work”;
> 
> where you “could” catch const char * and do strcmp on the message. You can, 
> the standard allows it, but it’s bad.

I disagree here, you are exaggerating. I see what you mean. But it
often happens that you have a specific error, like 'WriteError'. Then
you write tests and test specific errors and the way they are handled.
But the exception can still represent various errors like "broken
pipe", "permission denied" etc. And ideally you want to make sure in
the test the 'type' of the error is correct. So in this case, you can
make 'errno' a member of 'WriteError' and check that. There are cases
where there is no standard enumeration of the error types. Then you
either go all the way and write the enumeration yourself or just settle
with the error messages. In the end, it is actually the error message
what matters most to the user that is getting it.

I am of course not saying that checking the type isn't much better, it
is.

> > I certainly would not introduce specific error classes only for the sake of 
> > the tests.
> 
> Sorry, straw-man. I gave four reasons, I could list quite a few more if you 
> insist on that misguided suggestion of throwing runtime_error. So it’s 
> definitely not “only 

Re: [Spice-devel] [PATCH v2 22/24] Throw exception in case of write failure

[Christophe de Dinechin]

> On 22 Feb 2018, at 14:12, Lukáš Hrázký  wrote:
> 
> Apart from what's below, I'm also thinking if it wouldn't be better to
> leave this addition of exception classes for later and concentrate on
> finishing what's already present in this big series. Adding more stuff
> to it doesn't really speed things up :/
> 
> I think write_all() throwing a runtime_error as of now is surely no biggie.

It is a biggie. I hoped that your interest in writing unit tests would make it 
obvious.

It’s big enough to be E13 in the C++ Core Guidelines, 
https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Re-exception-types

"The standard-library classes derived from exception should be used only as 
base classes or for exceptions that require only "generic" handling. Like 
built-in types, their use could clash with other people's use of them."

Let me stand my ground here. I will not throw runtime_error in application code 
I write. I personally consider this “lazy”. I strongly invite you to not do it 
for the reasons already exposed, and if you don’t, I will propose patches to 
fix what I see as a design bug.


> On Thu, 2018-02-22 at 12:27 +0100, Christophe de Dinechin wrote:
>>> On 22 Feb 2018, at 10:42, Lukáš Hrázký  wrote:
>>> 
>>> On Wed, 2018-02-21 at 18:46 +0100, Christophe de Dinechin wrote:
 From: Christophe de Dinechin 
 
 This also introduces the spice::streaming_error::Error class, which we can 
 reuse
 later as a base class for all agent-specific errors. This class provides a
 formatted 'message()' class that returns a string, making it easier to 
 format
 errors without allocating memory at throw-time.
>>> 
>>> I am not at all convinced this approach has any advantages.
>> 
>> I realize that I only shared the benefits in a private message, not on-list. 
>> So here:
>> 
>> The benefit I see in this approach are
>> 
>> 1. there is no alloc at the throw point,
> 
> I don't see this as a benefit in itself, are there any implications I’m 
> missing?

At the throw point, the maximum amount of resources is allocated, and throwing 
the wrong exception will do the maximum amount of damage In particular, if you 
want to retry on a write error or do some specific cleanup and rethrow, getting 
a bad_alloc will kill these possibilities.

At the catch point, you have already discriminated what you could deal with, 
you were able to securely do retries or anything you wanted to do with 
exceptions you caught, and if after everything else fails, you get an error 
while printing the error message, that’s the minimum amount of possible damage.

But as I already wrote, if that’s so much of a concern, we can rewrite the 
message-generation code so that it never allocates.

> 
>> 2. the catch point can extract a specific exception type and its args (e.g. 
>> for unit testing)
> 
> This is true, but as I said, I consider this useful only for specific cases. 
> Often you don't care that much to differentiante.

This “you” is not me. I certainly do care that I can discriminate my exceptions.

> In unit tests, you can also check the error message.

You could, does not make it a good idea. IMO, it’s about as bad as

throw “That did not work”;

where you “could” catch const char * and do strcmp on the message. You can, the 
standard allows it, but it’s bad.

> I certainly would not introduce specific error classes only for the sake of 
> the tests.

Sorry, straw-man. I gave four reasons, I could list quite a few more if you 
insist on that misguided suggestion of throwing runtime_error. So it’s 
definitely not “only for the sake of the tests”.

> 
>> 3. formatting is in the class, rather than ad-hoc for every single throw
> 
> I find this partially benefitial for certain classes of exceptions.

We have a good example here, where instead of having to strerror(errno) at 
every place, we let the class do it.

As usual, it’s only one of many. This approach will only get better over time…

> By partially I mean, that while it puts the formatting in one place, it adds 
> the boilerplate of the class around it, so LOC for LOC, it's not that great 
> for small number of throws.

LOC is not my reference metric. As far as typing goes, this discussion is 
already well beyond the code in the patch :-)

> 
>> 4. We can distinguish errors we “expect” (e.g. Error and derived) from those 
>> we expect “less” (e.g. runtime_error).
> 
> I think this ir more or less 2.?

No, not at all. We expect write errors, we can do something about it, e.g. 
close the channel, reopen, etc.

We should never *expect* a runtime_error, it’s the standard library telling us 
something bad happened, e.g. you misused one of the standard containers. It’s 
more the result of a stray pointer in your code or unchecked input arguments 
than from bogus user input.

In other words, WriteError is a “normal” behavior of your program under 
external stress. 

Re: [Spice-devel] [PATCH v2 22/24] Throw exception in case of write failure

[Lukáš Hrázký]

Apart from what's below, I'm also thinking if it wouldn't be better to
leave this addition of exception classes for later and concentrate on
finishing what's already present in this big series. Adding more stuff
to it doesn't really speed things up :/

I think write_all() throwing a runtime_error as of now is surely no
biggie.


On Thu, 2018-02-22 at 12:27 +0100, Christophe de Dinechin wrote:
> > On 22 Feb 2018, at 10:42, Lukáš Hrázký  wrote:
> > 
> > On Wed, 2018-02-21 at 18:46 +0100, Christophe de Dinechin wrote:
> > > From: Christophe de Dinechin 
> > > 
> > > This also introduces the spice::streaming_error::Error class, which we 
> > > can reuse
> > > later as a base class for all agent-specific errors. This class provides a
> > > formatted 'message()' class that returns a string, making it easier to 
> > > format
> > > errors without allocating memory at throw-time.
> > 
> > I am not at all convinced this approach has any advantages.
> 
> I realize that I only shared the benefits in a private message, not on-list. 
> So here:
> 
> The benefit I see in this approach are
> 
> 1. there is no alloc at the throw point,

I don't see this as a benefit in itself, are there any implications I'm
missing?

> 2. the catch point can extract a specific exception type and its args (e.g. 
> for unit testing)

This is true, but as I said, I consider this useful only for specific
cases. Often you don't care that much to differentiante. In unit tests,
you can also check the error message. I certainly would not introduce
specific error classes only for the sake of the tests.

> 3. formatting is in the class, rather than ad-hoc for every single throw

I find this partially benefitial for certain classes of exceptions. By
partially I mean, that while it puts the formatting in one place, it
adds the boilerplate of the class around it, so LOC for LOC, it's not
that great for small number of throws.

> 4. We can distinguish errors we “expect” (e.g. Error and derived) from those 
> we expect “less” (e.g. runtime_error).

I think this ir more or less 2.?

> Which ones do you disagree with?

As you can see, I don't disagree in a black and white fashion. I think
I just want to avoid having 10+ different error classes with all the
noise it brings, it seems negatives outweight positives for me. Yes,
been there, done that.

> > How exactly makes it easier to format messages?
> 
> Consider for example if you wanted to use a printf-style interface for 
> formatting the message.
> 
> Two use cases:
> 1. Localization (printf is better than appending strings, allows for 
> reordering)
> 2. Flight recorder

If we have such a need, then sure, let's do it.

> > This means we would need classes
> > for all kinds of errors throughout the agent (unless you still want to
> > use runtime_errors in some places).
> 
> You don’t “need to”, you “want to”. It’s important to be able to discriminate 
> between classes of error. For exceptions, the exception class plays the same 
> role as errno values. If you would return the same errno, you can throw the 
> same class. Otherwise, you shouldn’t.

At current state of things, I don't want :) I only want to distinguish
error classes when I need to. Otherwise, spare me the churn :)

> > While, as I said, it makes sense to me in some places, in others I find it 
> > unnecessary work and code bloat (for lack of better word to express myself).
> 
> There are cases were we can reuse the same exception class, but using 
> runtime_error is not that class.
> 
> > 
> > > Signed-off-by: Christophe de Dinechin 
> > > ---
> > > src/spice-streaming-agent.cpp | 65 
> > > ---
> > > 1 file changed, 36 insertions(+), 29 deletions(-)
> > > 
> > > diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> > > index 4aae2cb..a789aad 100644
> > > --- a/src/spice-streaming-agent.cpp
> > > +++ b/src/spice-streaming-agent.cpp
> > > @@ -59,15 +59,30 @@ static uint64_t get_time(void)
> > > 
> > > }
> > > 
> > > +class Error : public std::runtime_error
> > > +{
> > > +public:
> > > +Error(const char *msg): runtime_error(msg) {}
> > > +virtual std::string message() { return what(); }
> > > +};
> > > +
> > 
> > I think introducing this class would warrant a separate commit.
> 
> I don’t like introducing a class without its use case, you can’t test it.

True that.

> > Also, it could go in it's own file straight away.
> 
> Yes. 
> 
> > 
> > > class Stream
> > > {
> > > typedef std::set codecs_t;
> > > 
> > > public:
> > > -class WriteError : public std::runtime_error
> > > +class WriteError final : public Error
> > > {
> > > public:
> > > -WriteError(const char *msg): runtime_error(msg) {}
> > > +WriteError(const char *msg, const char *operation, int 
> > > saved_errno)
> > 
> > The class is called WriteError, but this signature is apparently meant for 
> > an arbitrary 

Re: [Spice-devel] [PATCH v2 22/24] Throw exception in case of write failure

[Christophe de Dinechin]

> On 22 Feb 2018, at 10:42, Lukáš Hrázký  wrote:
> 
> On Wed, 2018-02-21 at 18:46 +0100, Christophe de Dinechin wrote:
>> From: Christophe de Dinechin 
>> 
>> This also introduces the spice::streaming_error::Error class, which we can 
>> reuse
>> later as a base class for all agent-specific errors. This class provides a
>> formatted 'message()' class that returns a string, making it easier to format
>> errors without allocating memory at throw-time.
> 
> I am not at all convinced this approach has any advantages.

I realize that I only shared the benefits in a private message, not on-list. So 
here:

The benefit I see in this approach are

1. there is no alloc at the throw point,
2. the catch point can extract a specific exception type and its args (e.g. for 
unit testing)
3. formatting is in the class, rather than ad-hoc for every single throw
4. We can distinguish errors we “expect” (e.g. Error and derived) from those we 
expect “less” (e.g. runtime_error).

Which ones do you disagree with?


> How exactly makes it easier to format messages?

Consider for example if you wanted to use a printf-style interface for 
formatting the message.

Two use cases:
1. Localization (printf is better than appending strings, allows for reordering)
2. Flight recorder


> This means we would need classes
> for all kinds of errors throughout the agent (unless you still want to
> use runtime_errors in some places).

You don’t “need to”, you “want to”. It’s important to be able to discriminate 
between classes of error. For exceptions, the exception class plays the same 
role as errno values. If you would return the same errno, you can throw the 
same class. Otherwise, you shouldn’t.


> While, as I said, it makes sense to me in some places, in others I find it 
> unnecessary work and code bloat (for lack of better word to express myself).

There are cases were we can reuse the same exception class, but using 
runtime_error is not that class.

> 
>> Signed-off-by: Christophe de Dinechin 
>> ---
>> src/spice-streaming-agent.cpp | 65 
>> ---
>> 1 file changed, 36 insertions(+), 29 deletions(-)
>> 
>> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
>> index 4aae2cb..a789aad 100644
>> --- a/src/spice-streaming-agent.cpp
>> +++ b/src/spice-streaming-agent.cpp
>> @@ -59,15 +59,30 @@ static uint64_t get_time(void)
>> 
>> }
>> 
>> +class Error : public std::runtime_error
>> +{
>> +public:
>> +Error(const char *msg): runtime_error(msg) {}
>> +virtual std::string message() { return what(); }
>> +};
>> +
> 
> I think introducing this class would warrant a separate commit.

I don’t like introducing a class without its use case, you can’t test it.

> Also, it could go in it's own file straight away.

Yes. 

> 
>> class Stream
>> {
>> typedef std::set codecs_t;
>> 
>> public:
>> -class WriteError : public std::runtime_error
>> +class WriteError final : public Error
>> {
>> public:
>> -WriteError(const char *msg): runtime_error(msg) {}
>> +WriteError(const char *msg, const char *operation, int saved_errno)
> 
> The class is called WriteError, but this signature is apparently meant for an 
> arbitrary operation that uses errno.

No. It’s for a write error as the name indicates. I could have a ReadError with 
the same signature, and if I really have both, have an IOError base with two 
trivial derived classes.

If at some point we want to deal with such errors, how we reconnect is likely 
to differ for read and write errors. Same for unit testing.


> 
>> +: Error(msg), operation(operation), saved_errno(saved_errno) {}
>> +std::string message()
>> +{
>> +return Error::message() + " in " + operation + ": " + 
>> strerror(saved_errno);
> 
> Ok, but isn't this actually creating the problem you described in an earlier 
> email?

It is, which is, as I pointed out, the reason the standard stuck with const 
char *.

But in our application, at the point I’m causing the potential problem, cleanup 
already happened. In other words, I’m moving allocations at the place where 
they are most likely to succeed.

BTW, if we are so out of memory that this will cause a bad_alloc, then iostream 
itself may also throw it, so short of going the long way and using ‘write’, we 
can’t do much better.

> You construct the error string during the exception handling, thus if this 
> throws an exception (as it can), you get a terminate?

The (only safe) alternative is to format the message in a pre-allocated static 
buffer using snprintf, and to print it with ::write to avoid anything that can 
allocate buffers. A bit overkill IMO.

> 
> Lukas
> 
>> +}
>> +private:
>> +const char *operation;
>> +int saved_errno;
>> };
>> 
>> public:
>> @@ -95,18 +110,11 @@ public:
>> int read_command(bool blocking);
>> 
>> template 

Re: [Spice-devel] [PATCH v2 22/24] Throw exception in case of write failure

[Lukáš Hrázký]

On Wed, 2018-02-21 at 18:46 +0100, Christophe de Dinechin wrote:
> From: Christophe de Dinechin 
> 
> This also introduces the spice::streaming_error::Error class, which we can 
> reuse
> later as a base class for all agent-specific errors. This class provides a
> formatted 'message()' class that returns a string, making it easier to format
> errors without allocating memory at throw-time.

I am not at all convinced this approach has any advantages. How exactly
makes it easier to format messages? This means we would need classes
for all kinds of errors throughout the agent (unless you still want to
use runtime_errors in some places). While, as I said, it makes sense to
me in some places, in others I find it unnecessary work and code bloat
(for lack of better word to express myself).

> Signed-off-by: Christophe de Dinechin 
> ---
>  src/spice-streaming-agent.cpp | 65 
> ---
>  1 file changed, 36 insertions(+), 29 deletions(-)
> 
> diff --git a/src/spice-streaming-agent.cpp b/src/spice-streaming-agent.cpp
> index 4aae2cb..a789aad 100644
> --- a/src/spice-streaming-agent.cpp
> +++ b/src/spice-streaming-agent.cpp
> @@ -59,15 +59,30 @@ static uint64_t get_time(void)
>  
>  }
>  
> +class Error : public std::runtime_error
> +{
> +public:
> +Error(const char *msg): runtime_error(msg) {}
> +virtual std::string message() { return what(); }
> +};
> +

I think introducing this class would warrant a separate commit. Also,
it could go in it's own file straight away.

>  class Stream
>  {
>  typedef std::set codecs_t;
>  
>  public:
> -class WriteError : public std::runtime_error
> +class WriteError final : public Error
>  {
>  public:
> -WriteError(const char *msg): runtime_error(msg) {}
> +WriteError(const char *msg, const char *operation, int saved_errno)

The class is called WriteError, but this signature is apparently meant
for an arbitrary operation that uses errno.

> +: Error(msg), operation(operation), saved_errno(saved_errno) {}
> +std::string message()
> +{
> +return Error::message() + " in " + operation + ": " + 
> strerror(saved_errno);

Ok, but isn't this actually creating the problem you described in an
earlier email? You construct the error string during the exception
handling, thus if this throws an exception (as it can), you get a
terminate?

Lukas

> +}
> +private:
> +const char *operation;
> +int saved_errno;
>  };
>  
>  public:
> @@ -95,18 +110,11 @@ public:
>  int read_command(bool blocking);
>  
>  template 
> -bool send(PayloadArgs... payload)
> +void send(PayloadArgs... payload)
>  {
>  Message message(payload...);
>  std::lock_guard stream_guard(mutex);
> -size_t expected = message.size(payload...);
> -size_t written = message.write(*this, payload...);
> -bool result = written == expected;
> -if (!result) {
> -syslog(LOG_ERR, "sent only %zu bytes out of %zu", written, 
> expected);
> -throw WriteError("Unable to write complete packet");
> -}
> -return result;
> +message.write(*this, payload...);
>  }
>  
>  size_t write_all(const char *what, const void *buf, const size_t len);
> @@ -151,9 +159,9 @@ struct FormatMessage : Message FormatMessage>
>  {
>  return StreamMsgFormat{ .width = w, .height = h, .codec = c, 
> .padding1 = {} };
>  }
> -size_t write(Stream , unsigned w, unsigned h, uint8_t c)
> +void write(Stream , unsigned w, unsigned h, uint8_t c)
>  {
> -return stream.write_all("FormatMessage", this, sizeof(message_t));
> +stream.write_all("FormatMessage", this, sizeof(message_t));
>  }
>  };
>  
> @@ -170,10 +178,10 @@ struct FrameMessage : Message FrameMessage>
>  {
>  return StreamMsgData();
>  }
> -size_t write(Stream , const void *frame, size_t length)
> +void write(Stream , const void *frame, size_t length)
>  {
> -return stream.write_all("FrameMessage header", this, 
> sizeof(message_t))
> -+  stream.write_all("FrameMessage frame", frame, length);
> +stream.write_all("FrameMessage header", this, sizeof(message_t));
> +stream.write_all("FrameMessage frame", frame, length);
>  }
>  };
>  
> @@ -208,11 +216,11 @@ struct X11CursorMessage : Message X11CursorMessage>
>  .data = { }
>  };
>  }
> -size_t write(Stream , XFixesCursorImage *cursor)
> +void write(Stream , XFixesCursorImage *cursor)
>  {
>  unsigned pixel_size = pixel_count(cursor) * sizeof(uint32_t);
> -return stream.write_all("X11CursorMessage header", this, 
> sizeof(message_t))
> -+  stream.write_all("X11CursorMessage pixels", pixels.get(), 
> pixel_size);
> +