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: handle null response from Eth server #287

Merged
merged 5 commits into from
Sep 2, 2024

Conversation

webwarrior-ws
Copy link
Contributor

Introduce WeirdNullResponseException type and raise it when response for balance request for Ethereum returns json with "result" field value of null. This way server will be marked as faulty instead of crashing the application.

Fixes #282

@webwarrior-ws webwarrior-ws force-pushed the weird-null-response-exn branch 2 times, most recently from dd9edad to 88e1be4 Compare August 19, 2024 09:22
@webwarrior-ws
Copy link
Contributor Author

I've added null checks to other Eth requests. There are 2 places that already do null checks, but raise generic Exception:
https://github.com/webwarrior-ws/geewallet/blob/88e1be44b9c98f17e84c9337d21eb91939bb1e9e/src/GWallet.Backend/Ether/EtherServer.fs#L504
https://github.com/webwarrior-ws/geewallet/blob/88e1be44b9c98f17e84c9337d21eb91939bb1e9e/src/GWallet.Backend/Ether/EtherServer.fs#L599

Should I also convert them to the new exception type?

@knocte
Copy link
Member

knocte commented Aug 19, 2024

Yes

@knocte
Copy link
Member

knocte commented Aug 21, 2024

Backend: add another Eth RPC error type

Backend: handle error 408 in Eth code

When you say "In Eth code" you are defining a scope. We already have conventions in the way we write git commit messages to specify what part of the code you are modifying: it is the scope, the first part of the commit message title, before the colon. The scope can be more specific than the one you wrote, thanks to the fact that we also allow subscopes. So, replace Backend: with Backend/Ether: to not need to write things like "In Eth code". Thanks to this, you won't need to many characters to specify where in the code you're changing stuff. And thanks to having less characters in the title, you can be more specific about what you're doing. For example, I'd prefer handle/retry instead of just handle, as it's more specific about what the code will really do after your commit. Last but not least, given that both the commits above are doing a very similar thing, let's make their commit titles as similar as possible.

@knocte
Copy link
Member

knocte commented Aug 21, 2024

Last but not least, given that both the commits above are doing a very similar thing, let's make their commit titles as similar as possible.

You ignored the sentence above.

BTW let's add an additional final commit that refactors some stuff: when reviewing this diff I saw a couple of null checks that looked weird:

if foo <> null then...
if Object.ReferenceEquals(null, foo) then
...

Let's replace both of those types (all occurrences) with isNull.

@knocte
Copy link
Member

knocte commented Sep 2, 2024

Let's rather make this PR target the stable branch.

@webwarrior-ws webwarrior-ws changed the base branch from master to stable September 2, 2024 08:10
@webwarrior-ws
Copy link
Contributor Author

rebased on stable

@knocte
Copy link
Member

knocte commented Sep 2, 2024

Two last things to do here before I merge this:

  • Some commit messages contain the text WeirdNullResponseException or something, when we have decided to rename that.
  • Please change one commit title to say "HTTP error 408", not just "error 408".

Introduce AbnormalNullValueInJsonResponseException type and
raise it when response for balance request for Ethereum returns
json with "result" field value of `null`. This way server will
be marked as faulty instead of crashing the application.

Fixes nblockchain#282
Raise WeirdNullResponseException on all possible null responses
or responses that contain null value from Ethereum servers.
Add code -39000 (UnparsableResponseType) to RpcErrorCode
enum and process it since this error was encountered during
integration testing.
Handle/retry HTTP error 408 (request timeout) in error
handling code (raise ServerTimedOutException).
Make all null checks use isNull function as it's the most
reliable way to test if value is null.
@knocte knocte merged commit c40b129 into nblockchain:stable Sep 2, 2024
19 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.

Strange null reference exception in Ether code
2 participants