Skip to content

Conversation

@munna0908
Copy link

No description provided.

@munna0908 munna0908 requested a review from dryajov March 27, 2025 16:11
@munna0908 munna0908 self-assigned this Mar 27, 2025
return failure(err.msg)
return success()
method delete*(self: LevelDbDatastore, keys: seq[Key]): Future[
?!void] {.async.} =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 This comment is likely beyond the scope of this PR, but we need to address this at some point (probably in a separate PR). I know you were amending what was already there, so definitely not something you have done. This is more for general information.

We should always try to annotate async pragmas with raises to inform the compiler and callers of potential exceptions that can be raised. This is useful so that callers can distinguish between exceptions being raised in a try/except block and potentially perform different operations depending on which exception was raised, eg

try:
  await key.read()
  await key.delete()
  key.removeFromListOfKeysToDelete()
except ReadError as e:
  # already doesn't exist, remove from deletion list
  key.removeFromListOfKeysToDelete()
except DeleteError as e
  # failed to delete, keep on deletion list, but may keep happening
  error "failed to delete", error = e.msg

is much more informative than:

try:
  await key.read()
  await key.delete()
  key.removeFromListOfKeysToDelete()
except CatchableError as e:
  # here we don't know which module raised the exception, and can only tell by inspecting the error message
  if "read failed" in e.msg:
    # already doesn't exist, remove from deletion list
    key.removeFromListOfKeysToDelete()
  elif "delete failed" in e.msg:
    # failed to delete, keep on deletion list, but may keep happening
    error "failed to delete", error = e.msg
  else:
    error "unknown error", error = e.msg

However, the operations in this module use return failure("some string") which returns a CatchableError instead of a module-specific and operation-specific exception, which means that the signatures in this module would simply have {.async: (raises: [CatchableError]).}. This does not give the caller much flexibility, where their only choice is wrap the calls in a try/except CatchableError and distinguish which exception was raised based on the error message.

So, we should be aiming to have module-specific errors as a base, and operation-specific errors as the raised exception, ie

type
  LevelDbDatastoreError = object of DatastoreError
  LevelDbReadError = object of LevelDbDatastoreError
  LevelDbDeleteError = object of LevelDbDatastoreError

method delete*(self: LevelDbDatastore, key: Key): Future[?!void] {.async: (raises: [LevelDbDeleteError]).} =
  try:
    #...
  except LevelDbException as e: 
    return failure newException(LevelDbDeleteError, "LevelDbDatastore.delete batch exception: " & e.msg, e)

This allows callers the flexibility to handle all exceptions from the module with eg try/except LevelDbDatastoreError, or operation-specific exceptions, eg try/except LevelDbDeleteError.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emizzle We have an active PR, Will accommodate your suggestions as part of that PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I misspoke a bit earlier. Since we're returning a Result which captures the exceptions, the raises pragma should only raise CancelledError, but the remainder of what I described should work.

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.

3 participants