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

chore: return errors using channels and not embedded in result type #10527

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

gammazero
Copy link
Contributor

Asynchronous functions should return errors over channels instead of embedding the error in the result type.

Closes #9974

@gammazero gammazero marked this pull request as ready for review October 2, 2024 17:15
@gammazero gammazero requested a review from a team as a code owner October 2, 2024 17:15
@gammazero gammazero added the skip/changelog This change does NOT require a changelog entry label Oct 2, 2024
// }
// fmt.Println("Dir name:", dirEnt.Name)
// }
func LsIter(ctx context.Context, api UnixfsAPI, p path.Path, opts ...options.UnixfsLsOption) iter.Seq2[DirEntry, error] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is implemented to operate on any UnixfsAPI since their Ls should all work the same. Is this a good assumption / implementation, or should each UnixfsAPI provide its own LsIter?

@gammazero gammazero added the need/analysis Needs further analysis before proceeding label Oct 22, 2024
@gammazero gammazero requested a review from lidel October 25, 2024 18:17
@hsanjuan
Copy link
Contributor

hsanjuan commented Nov 5, 2024

LGTM although improvement is marginal if this was already working code, with the side effect of altering CoreAPI (not sure if anyone builds anything on top of it other than Kubo itself).

I personally don't like the new pattern much more than the old one. Something like pin.Ls(...) (chan, chan) launches a goroutine and returns a channel:

  • The channel is chosen to be unbuffered even though it has no idea what the user wants it for.
  • Goroutine is hidden inside so that the user-code looks sync when it's async. Context management becomes important. One must not forget to finish reading or cancelling etc.

For me, the best pattern would be APIs like pin.Ls(chan Pins) err, where the user passes the channel on which they want the results written. The Ls function is all sync, it writes pins to the given channel, and when done closes the channel and sets the error. This forces goroutine/channel management fully onto the caller, which can choose how to buffer things etc. i.e.:

   out := make(chan Pins, 1000)
   make errCh := make(chan error, 1)
   go func() {
     defer close(errCh)
     errCh <- pin.Ls(ctx, out)
   }
   for p := range out {
     printPin(p)
   }
   err = <- errCh
   return err

The implementation of pin.Ls() does not need to allocate goroutines or channels, it can work fully in sync-mode by writing to a channel until something fails or its done, and the user can choose whether it wants to buffer or not.

Anyways this is just passing work around our own internal code, so it's all mostly the same.

Comment on lines 206 to 211
errOut := make(chan error, 1)
errOut <- err
close(errOut)
out := make(chan coreiface.DirEntry)
close(out)
return out, errOut
Copy link
Contributor

Choose a reason for hiding this comment

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

This kind of blocks are very ugly and are needed all over the place. Could we live with returning nil directly for the out channel?

@gammazero
Copy link
Contributor Author

Thank you for the review. This PR was a much as a exploration to find a more preferable pattern as to satisfy the original issue. I really agree with

it can work fully in sync-mode by writing to a channel until something fails or its done, and the user can choose whether it wants to buffer or not.

and also, let the caller decide whether or not the function should be called asynchronously.

The one benefit this PR does have is not having to wait on both ctx and err chan in case err chan is not read, but that is not really important with regart to the above statement. I will take another pass over this to incorporate your feedback.

@lidel lidel removed their assignment Nov 12, 2024
Asynchronous functions should return errors over channels instead of embedding the error in the result type.

Closes #9974
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/analysis Needs further analysis before proceeding skip/changelog This change does NOT require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kubo/client/rpc: use error channels
3 participants