Backend(LN): catch NOnionException TorOperations

[parhamsaremi]

Catch the NOnionException after the Retries have finished and the problem still exists. We catch the exception and return an error.

Fixes #181
Fixes #182

[knocte]

@aarani please review

[knocte]

Fixes #181

Are you sure? I think this is false ^

[knocte]

@aarani please review

ping

[aarani]

LGTM

[knocte]

LGTM

Are you sure? Why are we replacing specific exception types with generic exception types in this PR? I don't understand.

[parhamsaremi]

Fixes #181

Are you sure? I think this is false ^

Actually, I think this PR with the PR that I made to NOnion would solve this problem. In that PR, I converted the System.Exception to IOException and this one catches the NOntion Exceptions.

[parhamsaremi]

LGTM

Are you sure? Why are we replacing specific exception types with generic exception types in this PR? I don't understand.

When we catch NOnionExceptions in TorOperations, we have to return them as Result<> type. Since the code in those files was operating with SocketException, I had to change the types to a more generic one, so that I could be able to return both of these errors 🤔

[knocte]

Actually, I think this PR with the PR that I made to NOnion would solve this problem. In that PR, I converted the System.Exception to IOException and this one catches the NOntion Exceptions.

So?

[knocte]

Since the code in those files was operating with SocketException

I don't understand the reasoning here. The NOnion PR that was recently merged was done so that we didn't need to catch generic exceptions. And this PR introduces generic exceptions?

[parhamsaremi]

Actually, I think this PR with the PR that I made to NOnion would solve this problem. In that PR, I converted the System.Exception to IOException and this one catches the NOntion Exceptions.

So?

Should I link that PR too in the description? I didn't know where I should put the link! should I put it in the body? or in the footer?

[parhamsaremi]

Since the code in those files was operating with SocketException

I don't understand the reasoning here. The NOnion PR that was recently merged was done so that we didn't need to catch generic exceptions. And this PR introduces generic exceptions?

We don't catch generic exceptions 🤔 we are just catching NOnionException but the type of that should have changed so that We could pass both NOnionException and SocketException to the upper level functions. If an System.Excption is thrown from the NOninon, we are not catching it.

[knocte]

We don't catch generic exceptions 🤔 we are just catching NOnionException but the type of that should have changed so that We could pass both NOnionException and SocketException to the upper level functions. If an System.Excption is thrown from the NOninon, we are not catching it.

If you need a type to represent Exception A or Exception B, make a new DU. Do not use base class System.Exception, ever. It invites bad practices.

[knocte]

Should I link that PR too in the description? I didn't know where I should put the link! should I put it in the body? or in the footer?

Why would you want to link that PR in the description?

[parhamsaremi]

We don't catch generic exceptions thinking we are just catching NOnionException but the type of that should have changed so that We could pass both NOnionException and SocketException to the upper level functions. If an System.Excption is thrown from the NOninon, we are not catching it.

If you need a type to represent Exception A or Exception B, make a new DU. Do not use base class System.Exception, ever. It invites bad practices.

Hmmm sounds good, I'll work on it 🤔

[knocte]

Hmmm sounds good, I'll work on it

Actually, are we sure we want to hardcode SocketException in geewallet? Shouldn't NOnion wrap that so that we don't have to?

[parhamsaremi]

Should I link that PR too in the description? I didn't know where I should put the link! should I put it in the body? or in the footer?

Why would you want to link that PR in the description?

My understanding was that these two PRs are connected and both of them Fix the issue with each other 🤔

[parhamsaremi]

Hmmm sounds good, I'll work on it

Actually, are we sure we want to hardcode SocketException in geewallet? Shouldn't NOnion wrap that so that we don't have to?

I'm not familiar with SocketExceptions, but I think this idea sounds cleaner 🤔

[knocte]

My understanding was that these two PRs are connected and both of them Fix the issue with each other

That PR is already merged. If we merge this PR, the bug is still not fixed. You're still missing one important thing.

[knocte]

I'm not familiar with SocketExceptions, but I think this idea sounds cleaner

Then try it. (I'm not familiar with them either, I just receive them sometimes and I need to make my code protected against them. I've never looked into their source code.)

[parhamsaremi]

My understanding was that these two PRs are connected and both of them Fix the issue with each other

That PR is already merged. If we merge this PR, the bug is still not fixed. You're still missing one important thing.

right 👍

[parhamsaremi]

My understanding was that these two PRs are connected and both of them Fix the issue with each other

That PR is already merged. If we merge this PR, the bug is still not fixed. You're still missing one important thing.

I'll work on updating the NOnion version.

[parhamsaremi]

Hmmm sounds good, I'll work on it

Actually, are we sure we want to hardcode SocketException in geewallet? Shouldn't NOnion wrap that so that we don't have to?

For now, I updated the code such that we won't have the problem of generic Exception by defining the case for NOnionException. I think the SocketException needs more investigation.

[knocte]

The CI of commit titled Backend,Tests: update NOnion version is red because it doesn't even build. It doesn't make sense to separate it from the next commit which fixes the build.

[parhamsaremi]

The CI of commit titled Backend,Tests: update NOnion version is red because it doesn't even build. It doesn't make sense to separate it from the next commit which fixes the build.

Sure I'll squash them right now.

[knocte]

When merging the last 2 commits, you discarded the message where you explained the use of new keyword, why???

[knocte]

^ Both commits fix bug 181?

[parhamsaremi]

^ Both commits fix bug 181?

My bad. Technically both of them are needed. But I should have only mentioned it in the second one.

[parhamsaremi]

When merging the last 2 commits, you discarded the message where you explained the use of new keyword, why???

oops I'll fix this right away.

[knocte]

When merging the last 2 commits, you discarded the message where you explained the use of new keyword, why???

You didn't read this?

[parhamsaremi]

When merging the last 2 commits, you discarded the message where you explained the use of new keyword, why???

I answered it and said it was my bad. I'm adding it right now. if you are fixating on the why, I'm not sure I think I might have just missed it when I was squashing two commits.

[knocte]

I answered it and said it was my bad. I'm adding it right now. if you are fixating on the why, I'm not sure I think I might have just missed it when I was squashing two commits.

Sorry it's just that I saw you addressing only 1 of my comments and pushing without addressing my 2nd comment. CI in lightning branch takes long, so if you're gonna push, adress all comments first without pushing in between, otherwise you clog GitHubActions with useless (obsolete) jobs that make the PR's CI take unnecessary longer.