search

greenaddress / abcore

ABCore - Android Bitcoin Core
http://abco.re
MIT License
164 stars 55 forks source link

Catch RPC exception on broadcastNetwork #108

Open caiofaustino opened 4 years ago

caiofaustino commented 4 years ago

If the exception happens the service stops responding. Catch and send error instead.

lvaccaro commented 4 years ago

Sorry, it is not really clear to me the desired behaviour: why do you handle internally the exception, breaking broadcastNetwork(), instead of propagate the exception to the caller (which just has try catch block and send a broadcastError)?

caiofaustino commented 4 years ago

I wanted it to just fail silently because the broadcastError triggers some logic on the MainActivitythat can call a stop to the ABCoreSerice when it shouldn't.

broadcastNetwork is only throwing IOException but the RPC client throws a BitcoinRPCException, so on my tests in some scenarios the Service would just stop there and not process further commands because it was waiting for that one to finish.

If I let it throw and call broadcastError, in some cases it would fall into a never starting cycle. Bitcoin starts, Tor is starting, then we request Tor status, but since Tor is still initialising the status fails, that causes the exception and broadcastError, then the MainActivity gets that and checks the unreliable Daemon status to see if it should stop the service, so it calls stopDaemonAndSetStatus which goes into another loop until it manages to force stop Tor and BTC. The whole thing ended up looking like "Staring...Stopped" every time you tried to start the service.

Since the "localonion" command is not critical, I think that even if it fails to get the current Tor status, doesn't mean there was an error in the BTC process or anything worth telling the UI that the error happened. It will just try again in 1 second anyway.

I'm thinking about a bigger refactor later, but I thought it would be more helpful to do smaller non intrusive fixes for now to at least get a baseline of how it should work.

lvaccaro commented 4 years ago

thanks, I reproduce the previously described case and that hack prevents to call the stopDaemonAndSetStatus.
On the other hand, maybe just ignoring localonion errors on rpc could not be safe enough, because there are two different cases handled by BitcoinRPCException based on response code: if response code == 500, it returns a broadcast intent, else it calls broadcastError (that is the case you described before).

In other words, testing the PR when I switch power core ON: broadcastNetwork() fails without replying (and also the following requests fail every second), without update the switch button to off.

caiofaustino commented 4 years ago

Thanks for looking into the PR btw, nice to feel like it's getting some progress.

I agree, maybe failing silently inside is not the safest approach. I'll change that.

What I don't really understand is why response code 500 (Internal Server Error) returns message "ok". The logic seems weird and I think it's only like that because it's too coupled with the logic on MainActivity.

Since the broadcastError triggers unexpected results, I think an empty BitcoinRPCException (null values and code 0) happens too often to trigger the broadcast and maybe cause the core service to stop, which is the behavior I',m trying to fix in the overall PRs. So I think ignoring it for now is the best approach until I can propose a wider change.

I'll update the PR, let me know what you think :)

lvaccaro commented 4 years ago

It looks better now, but broadcastPeerlist() fails with error code 0 and it should broadcast the error. If you try to open the peer page when core is off, the broadcastPeerlist() fails without error code 0, the catch ignore that case without replying, so in the PeerActivity the progressbar continue to progress and it is never refreshed.

I agree about refactoring all that exceptions.

caiofaustino commented 4 years ago

Hmm yeah I didn't check the peers list screen, I was only checking the ON/OFF toggle. I'll investigate it a bit more and try to decouple things a big.

caiofaustino commented 4 years ago

I'm having difficulty to troubleshoot this properly without merging the fix on #106 first. @lvaccaro can you help review that one first?

caiofaustino commented 4 years ago

I finished the tests here, to me it seems that the issues are solved by #106 + just catching the BitcoinRPCException here. There rest of the changes in this PR ended up being just minor code formatting and logging.

But yeah this depends on #106 to work properly.