Some exception refactoring

[lbonn]

Tests do not pass yet and it will probably need some changes in tuf-test-vectors as well.

[lbonn]

@patrickvacek Just checking on uptane vectors

{director,image_repo}root_rotation_version{skip,unchanged} return "Uptane::SecurityException: Version in Root metadata doesn't match the expected value". I suppose it's better than unmet threshold? I'll probably change it to a more specific exception anyway.

[pattivacek]

{director,image_repo}_root_rotation_version_{skip,unchanged} return "Uptane::SecurityException: Version in Root metadata doesn't match the expected value". I suppose it's better than unmet threshold? I'll probably change it to a more specific exception anyway.

"Version in Root metadata doesn't match the expected value" sounds correct to me. A lot of the errors expected by the vector tests are bogus because we were waiting on this refactoring before spending too much time getting everything just right. I think the "umet threshold" and a few others are common exceptions that really should be something more detailed.

[pattivacek]

I'll probably change it to a more specific exception anyway.

Just saw the RootVersionChainMismatch that you added. That looks like an improvement!

[lbonn]

"Version in Root metadata doesn't match the expected value" sounds correct to me. A lot of the errors expected by the vector tests are bogus because we were waiting on this refactoring before spending too much time getting everything just right. I think the "umet threshold" and a few others are common exceptions that really should be something more detailed.

Thanks, that's what I thought. I've opened a PR with the changes, the text is now "One Root version is not a strict increment of the preceding." (to not re-use a plain SecurityException). I guess we can discuss names and descriptions there advancedtelematic/tuf-test-vectors#71

[lbonn]

I also realized that removing "plain throws" like I did here #1654 was a mistake. When done in a catch clause, it rethrows the caught exception.

[pattivacek]

I also realized that removing "plain throws" like I did here #1654 was a mistake. When done in a catch clause, it rethrows the caught exception.

I don't think I understand the problem; that sounds like what I expected. What's wrong with rethrowing?

[lbonn]

I also realized that removing "plain throws" like I did here #1654 was a mistake. When done in a catch clause, it rethrows the caught exception.

I don't think I understand the problem; that sounds like what I expected. What's wrong with rethrowing?

Ah sorry it was unclear. Exactly, nothing wrong with rethrowing :).

try {
} catch (const std::exception& e) {
  throw;
}

is actually better than

try {
} catch (const std::exception& e) {
  throw e;
}

Because in the second case, we slice the original exception and loose the original context.

[pattivacek]

Already looks like a big improvement! What's the benefit of last_exception in SotaUptaneClient, though? And I'd like to make sure that @eu-smirnov is on board with this direction.

Because in the second case, we slice the original exception and loose the original context.

Oh crap, right. Good catch.

[pattivacek]

👍👍👍👍👍👍 This should've been done long ago!

[lbonn]

Already looks like a big improvement! What's the benefit of last_exception in SotaUptaneClient, though? And I'd like to make sure that @eu-smirnov is on board with this direction.

I tried to not change the external API that tried its best no to throw, so the last_exception is used to report the error reason as understood by tuf-test-vectors. There might be a better way to expose that through the Aktualizr API.

Basically my conclusion was that the way we constructed the API, asynchronous and sending events, using exceptions would be quite awkward and hard to handle (for example, we still want to report something to the backend even if an install fails because of a metadata check). For now I've chosen to reserve them for cases that are hard/impossible to recover from. Otherwise I'm afraid it would actually worsen the usability of the API.

EDIT: so to summarize, the objective right now is to let roll exceptions as far as possible and catch them at a small number of points in API boundaries for error reporting.

[pattivacek]

I tried to not change the external API that tried its best no to throw, so the last_exception is used to report the error reason as understood by tuf-test-vectors. There might be a better way to expose that through the Aktualizr API.

Basically my conclusion was that the way we constructed the API, asynchronous and sending events, using exceptions would be quite awkward and hard to handle (for example, we still want to report something to the backend even if an install fails because of a metadata check). For now I've chosen to reserve them for cases that are hard/impossible to recover from. Otherwise I'm afraid it would actually worsen the usability of the API.

EDIT: so to summarize, the objective right now is to let roll exceptions as far as possible and catch them at a small number of points in API boundaries for error reporting.

Okay, that makes sense to me. Even if we want to throw exceptions through the API, we would need to do the work in this PR first to make sure we know what we're throwing. I'm open to discussion on what to do from here.

[eu-siemann]

What prevents us from moving this try..catch block one level higher, to the downloadImages method and get rid of success return value?

[lbonn]

Because of the EcuDownloadCompleteReport and DownloadTarget event. I tried to move them out but then noticed that aktualizr-lite is calling downloadImage() directly, so it would miss these calls right now.

We'd have to make some changes there to fix this. I can add a comment.

[eu-siemann]

Right, I missed them somehow.

[eu-siemann]

Definitely looks like an improvement to me! I'm still unsure about a few places, where I left the comments, and about the idea of catching all exceptions before the API level. But we can discuss it later and it shouldn't prevent us from merging this PR.
It also would be cool if we could avoid

try {
  ...
} catch () {
  LOG_ERROR << "";
  throw;
}

blocks by having more information in the exception object, so we can move the prints higher in the call stack.

[lbonn]

It also would be cool if we could avoid

try {
  ...
} catch () {
  LOG_ERROR << "";
  throw;
}

blocks by having more information in the exception object, so we can move the prints higher in the call stack.

That's true that they are a bit ugly but I thought it's often useful to know the high level operation that caused the error. Sometimes we only have one caller but otherwise, I don't know how to log that better.

Or we define a macro (:smiley:)

[lbonn]

Should be ready to go