Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Async exception tracking #24582

Closed
wants to merge 5 commits into from
Closed

Conversation

nitely
Copy link
Contributor

@nitely nitely commented Dec 28, 2024

Somewhat inspired in chronos.

Goal:

  • Enable Exception Tracking for async. Using the native Nim exception tracking.
  • Enable passing error typed Futures around.
  • No runtime overhead.
  • ~Backward compatible.

How:

  • Await retrieves the raises list for the future and sets it as raises for the read call. If raises cannot be retrieved, then nothing is set, and the current behavior is kept (raises: [Exception]).
  • toFutureEx macro takes an async function call, and it returns the error typed future FutureEx[T, E] for passing it around.
  • Callbacks must not raise for this to work. It's possible to make this backward compatible, I think. Just deprecate the proc that takes a cb without raises: [], and cast it to untrack raises.
  • newFuture() usage in user code usually leads to inferred Exception at some point because read(Future) can raise anything. If the user wants granular raises, they must use FutureEx.

Todo:

  • tests
  • implement all, and, or FutureEx versions
  • implement fail FutureEx version; can it be done so it only accepts the typed exceptions?
  • fix all XXX
  • check all nimble packages using async compile

@nitely nitely force-pushed the async_error_tracking branch from 3f4b553 to 1ed9ea6 Compare January 7, 2025 01:11
@nitely
Copy link
Contributor Author

nitely commented Jan 7, 2025

I'm almost certain this cannot be done in a backward compatible way. A lot of (non-async) procs in asyncdispatch.nim create and return a newFuture[T], and so they raises: [Exception]. These procs would need to return a FutureEx[T, E] to make error tracking granular. One way forward is to create an asyncdispatch2.nim. Make all procs wrappers of the error typed versions, and deprecate them. But that's pretty bad, and a lot of work.

Maybe making FutureEx*[T, E] = ref object of Future[T] instead of FutureEx*[T, E] = distinct Future[T] would cause less breakage in practice. But asyncdispatch2.nim would still be needed because we cannot know how these are used outside of the nimble packages.

I do realize this is throwing the baby out with the bathwater. The user could handle the asyncdispatch Exception raises, and have sane error tracking in the rest of their code. But I don't see users requesting async error tracking either.

@nitely nitely closed this Jan 7, 2025
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.

1 participant