Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backend(,Tests): stop using binary serialization for exceptions #253

Merged
merged 1 commit into from
Feb 1, 2024

Conversation

knocte
Copy link
Member

@knocte knocte commented Jan 31, 2024

We had a need in the past to serialize exceptions (as they could happen in off-line mode, when running as a cold-storage device), so that they could be reported later when the device comes online, but exceptions can't be seralizated to JSON (as explained in [1]), so we ended up using binary serialization (hooking it up in this past commit[2]).

However, binary serialization is going away in .NET9[3] because of its potential security risk. Even though we doubt that for our use case we would be affected by this security vector, we:

  • Want to be prepared for the future.
  • Know that there were anyway edge cases where binary serialization was not actually working (e.g. see bug 240), and was causing crashes.

We explored the idea of contributing an IException interface to the 'sentry-dotnet' repo [4] (this library is the replacement of SharpRaven, see [5]), so that we can serialize exceptions easily in JSON, for later deserializing them and send them straight to Sentry's API for report purposes, however:

  • We found adding the IException overloads to be extremely complicated due to the sheer amount of unit tests and things that Sentry has, that would need to be modified.
  • Given the above, we thought it would be too much work, and too much risk of not being accepted upstream.
  • Even if the IException overloads were accepted, the approach would still be a leaky abstraction because the type of the exception cannot be properly represented in a hypothetical IException's property, so we were/would ending up with hacky things such as an IsAggregateException:bool property, for example. But why end here and not have more bool types for other exceptions?

Instead of the above nightmare we have decided to go for the simplest approach of all (the one that I should have done 4 years ago when I was initially solving this problem, to avoid any OVERENGINEERING): just use good old Exception.ToString() method! This method provides, not only the type of the exception and its .Message property, also all its inner exceptions recursively. This is GOOD ENOUGH.

Fixes #240

[1] 403d5c7
[2] 1f7b3b7
[3] https://twitter.com/SitnikAdam/status/1746874459640811575
[4] https://github.com/getsentry/sentry-dotnet
[5] #252

@knocte knocte force-pushed the wip/exceptionMarshallingWithoutOverEngineering branch 3 times, most recently from 55c4cef to 3cc1457 Compare January 31, 2024 08:59
@knocte knocte force-pushed the wip/exceptionMarshallingWithoutOverEngineering branch 8 times, most recently from cfcc772 to ebe7421 Compare February 1, 2024 11:27
We had a need in the past to serialize exceptions (as they could
happen in off-line mode, when running as a cold-storage device),
so that they could be reported later when the device comes
online, but exceptions can't be seralizated to JSON (as
explained in [1]), so we ended up using binary serialization
(hooking it up in this past commit[2]).

However, binary serialization is going away in .NET9[3] because
of its potential security risk. Even though we doubt that for
our use case we would be affected by this security vector, we:
- Want to be prepared for the future.
- Know that there were anyway edge cases where binary
serialization was not actually working (e.g. see bug 240), and
was causing crashes.

We explored the idea of contributing an IException interface to
the 'sentry-dotnet' repo [4] (this library is the replacement of
SharpRaven, see [5]), so that we can serialize exceptions easily
in JSON, for later deserializing them and send them straight to
Sentry's API for report purposes, however:
* We found adding the IException overloads to be extremely
complicated due to the sheer amount of unit tests and things
that Sentry has, that would need to be modified.
* Given the above, we thought it would be too much work, and too
much risk of not being accepted upstream.
* Even if the IException overloads were accepted, the approach
would still be a leaky abstraction because the type of the
exception cannot be properly represented in a hypothetical
IException's property, so we were/would ending up with hacky
things such as an IsAggregateException:bool property, for
example. But why end here and not have more bool types for
other exceptions?

Instead of the above nightmare we have decided to go for the
simplest approach of all (the one that I should have done 3ish
years ago when I was initially solving this problem, to avoid
any OVERENGINEERING): just use good old Exception.ToString()
method! This method provides, not only the type of the
exception and its .Message property, also all its inner
exceptions recursively. This is GOOD ENOUGH.

Fixes #240
Closes https://gitlab.com/nblockchain/geewallet/-/issues/174

[1] 403d5c7
[2] 1f7b3b7
[3] https://twitter.com/SitnikAdam/status/1746874459640811575
[4] https://github.com/getsentry/sentry-dotnet
[5] #252
@knocte knocte force-pushed the wip/exceptionMarshallingWithoutOverEngineering branch from ebe7421 to d3f0e56 Compare February 1, 2024 11:54
@knocte knocte merged commit 83d8fcf into master Feb 1, 2024
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem serializing HTTP exception
1 participant