fix: http unsubscribe

[marcinczenko]

Unsubscribing from a non-existent subscription as well as duplicate unsubscribe should have no effect (harmless).

[emizzle]

Good find!

[emizzle]

Another option is to move the try two lines up, since except CatchableError will catch KeyError. But the way you have it is also fine.

The main advantage of doing it this way is that they compiler will not think that KeyError can be leaked out from this proc (even though it shouldn't based on the hasKey usage). To go one step further, we could (and probably should) annotate the proc with {.async: (raises: [CancelledError]).} to indicate that no other exceptions, except CancelledError, could potentially be emitted by the proc.

[markspanbroek]

You could also use this construct from questionable to deal with the KeyError:

  if sub =? subscriptions.subscriptionMapping.?[id]:

[emizzle]

Interesting, so .? works even without Options or Results?

[marcinczenko]

I am still making mistakes when applying "questionables", but I like the conciseness of Mark's proposal. I will update it.

[marcinczenko]

Adding raises btw is also good thing to do I suppose. Perhaps we should do it always where proc is handling exceptions... But here it makes things more complicated. We need to modify signature of the base method, and the override for web sockets does not do any exceptions handling so it also throws CatchableError. Thus... perhaps we should not add raises to unsubscribe.

[marcinczenko]

I actually aways like in short methods like this to have try/catch on top-level. If we move try to the top, then the KeyError would be cough even without using the conditional. For the readability reasons I would still use the conditional.

[markspanbroek]

Interesting, so .? works even without Options or Results?

The .?[] syntax is a special syntax for indexing strings, arrays and tables: https://github.com/codex-storage/questionable/blob/main/questionable/indexing.nim

[emizzle]

Adding raises btw is also good thing to do I suppose. Perhaps we should do it always where proc is handling exceptions... But here it makes things more complicated. We need to modify signature of the base method, and the override for web sockets does not do any exceptions handling so it also throws CatchableError. Thus... perhaps we should not add raises to unsubscribe.

It's a good point. And also makes me think that we should be handling errors in WebSocketSubscriptions.unsubscribe the same way as we are here in PollingSubscriptions.unsubscribe (explicitly raising only CancelledError), but could do that in a different PR at some point later, so we can keep this changeset small.

[marcinczenko]

Yes, when we see it is best to record it. I will create a draft for this later.

So the difference between the two "unsubscribes" is the following:

# websockets
method unsubscribe*(subscriptions: WebSocketSubscriptions,
                   id: JsonNode)
                  {.async.} =
  subscriptions.callbacks.del(id)
  discard await subscriptions.client.eth_unsubscribe(id)

#http
method unsubscribe*(subscriptions: PollingSubscriptions,
                   id: JsonNode)
                  {.async.} =
  try:
    subscriptions.logFilters.del(id)
    subscriptions.callbacks.del(id)
    if sub =? subscriptions.subscriptionMapping.?[id]:
      subscriptions.subscriptionMapping.del(id)
      discard await subscriptions.client.eth_uninstallFilter(sub)
  except CancelledError as e:
    raise e
  except CatchableError:
    # Ignore if uninstallation of the filter fails. If it's the last step in our
    # cleanup, then filter changes for this filter will no longer be polled so
    # if the filter continues to live on in geth for whatever reason then it
    # doesn't matter.
    discard

So we have two different RPC calls: eth_unsubscribe for websockets and eth_uninstallFilter for http.
It is kind of tricky to get a good picture of what exceptions those two calls are throwing. Is it by any chance implementation dependent? Like depending which RPC provider or chain we are using? The docs are not really aligned about that...

I will look further to see if it is possible to find some common ground.
In general though, it is preferred that things like unsubscribe should not really throw anything. @markspanbroek see the CancelledError for http as a tolerable exception to that rule.

What is your opinion about it?

[emizzle]

Is it by any chance implementation dependent? Like depending which RPC provider or chain we are using?

I'm not sure, honestly. However, as you mentioned, regardless of what kind of error response is returned, we don't really need to re-raise or handle these errors for unsubscriptions. We should only re-raise CancelledError so that we can bubble cancellations.

[markspanbroek]

Thanks @marcinczenko, looks good to me