chore: fix async raises warnings #100
Conversation
| body | ||
| except CancelledError as error: | ||
| raise error | ||
| except CatchableError as error: |
There was a problem hiding this comment.
Why are we blindly converting any exception into a SubscriptionError, this obfuscates the actual error.
There was a problem hiding this comment.
I am just wrapping it, so it does not obfuscate it, you will get the original stack error if you ask for it. This is because json_rpc (eq. any eth_* calls) can throw CatchableError.
I just wanted to remove some warnings around async raises, I am happy to simply remove those completely 🤷♂️ Or do whatever you want, I just don't want to spend any big time on this, please.
There was a problem hiding this comment.
I just wanted to remove some warnings around async raises
What you're doing is hiding a potential underlying problem, not properly fixing it.
I was taking a closer look at the code and I'm not even sure where toErr comes from and what does it do. Do you?
There was a problem hiding this comment.
Do you?
Yes, it is coming from here
Line 13 in 04d3548
There was a problem hiding this comment.
This is just a way to "unify" the error coming from this module (Subscription) in order to communicate that these errors might come from this module.
There was a problem hiding this comment.
@dryajov I agree it's good not to overuse something like this, however calls to these RPC endpoints result in a generic JsonRpcError error regardless of what the error is. Other than RPC calls, there are very few exception raising paths. So this template is convenient for converting these errors to a module-level error.
@AuHau could you catch JsonRpcError specifically (instead of CatchableError) and see if it doesn't leak any other errors? There might be a KeyError here or there that could be caught separately, but I don't see it as really being of much benefit.
There was a problem hiding this comment.
If I change the CatchableError to JsonRpcError, the compiler will complain with this:
.../nim-ethers/ethers/providers/jsonrpc/subscriptions.nim(123, 40) Error: cast[type(eth_subscribe(subscriptions.client, "newHeads"))](chronosInternalRetFuture.internalChild).internalError can raise an unlisted exception: ref CatchableError
So this confirms what I was saying earlier - any RPC's eth_* call we have will raise CatchableError and that is what we need to work with. So I see 4 approaches:
- We add
CatchableErrorto the async raises. - We at least wrap it to
SubscriptionErrorand raise that. - We remove
raisesall together. - We do nothing.
I can't think of anything else. So @dryajov please pick. My favorite is the current approach - number 2.
There was a problem hiding this comment.
Ok, I see that we're wrapping the underlying error, that is good enough.
There was a problem hiding this comment.
I know we discussed this already, but just for posterity, it seems that the compiler always reports that a CatchableError is leaking, and never reports the derived type. As an example, there could be a KeyError being leaked, which is derived from CatchableError, but the compiler will still report that a CatchableError is leaking.
| convertErrorsToSubscriptionError: | ||
| subscriptions.callbacks.del(id) | ||
| discard await subscriptions.client.eth_unsubscribe(id) |
There was a problem hiding this comment.
I think we should raise [] from this proc, since we don't really care if unsubscribing fails. This will require a change to the base method as well, but the PollingSubscriptions override should mostly be handled already -- see #98 (comment) for some context.
| body | ||
| except CancelledError as error: | ||
| raise error | ||
| except CatchableError as error: |
There was a problem hiding this comment.
@dryajov I agree it's good not to overuse something like this, however calls to these RPC endpoints result in a generic JsonRpcError error regardless of what the error is. Other than RPC calls, there are very few exception raising paths. So this template is convenient for converting these errors to a module-level error.
@AuHau could you catch JsonRpcError specifically (instead of CatchableError) and see if it doesn't leak any other errors? There might be a KeyError here or there that could be caught separately, but I don't see it as really being of much benefit.
No description provided.