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

refactor(cmds): do not return errors embedded in result type #10527

Merged
merged 10 commits into from
Dec 3, 2024

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it doesn't hurt, but it not used?

@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
@gammazero gammazero requested a review from hsanjuan November 4, 2024 19:24
@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.

core/coreapi/unixfs.go Outdated Show resolved Hide resolved
@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
@gammazero gammazero marked this pull request as draft November 20, 2024 16:28
@gammazero gammazero force-pushed the use-error-channels branch 2 times, most recently from 0c8c900 to 22f399d Compare November 27, 2024 19:51
@gammazero gammazero marked this pull request as ready for review November 27, 2024 20:17
Copy link
Contributor

@hsanjuan hsanjuan left a comment

Choose a reason for hiding this comment

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

I think everything has become much nicer to read.

client/rpc/pin.go Outdated Show resolved Hide resolved
client/rpc/unixfs.go Outdated Show resolved Hide resolved
core/commands/pin/remotepin.go Outdated Show resolved Hide resolved
// }
// 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

Choose a reason for hiding this comment

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

I guess it doesn't hurt, but it not used?

@gammazero gammazero changed the title chore: return errors using channels and not embedded in result type chore: do not return errors embedded in result type Dec 3, 2024
@lidel lidel mentioned this pull request Dec 3, 2024
40 tasks
@lidel lidel changed the title chore: do not return errors embedded in result type refactor(cmds): do not return errors embedded in result type Dec 3, 2024
@lidel lidel merged commit 224d6a3 into master Dec 3, 2024
14 checks passed
@lidel lidel deleted the use-error-channels branch December 3, 2024 19:15
@lidel lidel restored the use-error-channels branch December 3, 2024 19:15
@lidel lidel deleted the use-error-channels branch December 3, 2024 19:15
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