Skip to content
This repository has been archived by the owner on Jun 19, 2023. It is now read-only.

feat: use error channels #62

Closed
wants to merge 2 commits into from
Closed

feat: use error channels #62

wants to merge 2 commits into from

Conversation

Stebalien
Copy link
Member

@Stebalien Stebalien commented May 6, 2020

Refactor the API to use error channels where appropriate.

Previously, asynchronous functions in this interface would return channels of "results" that can be either errors or values as shown below

type Result struct {
  Value ValueType
  Err error
}
func Request(context.Context) (results <-chan Result)

Unfortunately, there are several issues with this approach:

The basic problem with this approach is that it requires defining a new "result" type and checking if each value is either a result or error. This is hardly a show-stopper but it infects values with additional error fields.

However, the main issue with this approach is cancellation. If the context is canceled, the Request implementation has a few options:

  1. Close the results channel and walk away. Unfortunately, the caller will likely interpret this as "request competed" instead of "request canceled". To correctly use functions like this, the caller must check if the context after the channel closes.
  2. Leave the result channel open. In this case, the caller must select on its own context. This is a pretty sane approach, but it makes it impossible to safely use the results channel in a "for range" loop.
  3. Write the cancellation error to the results channel. This will ensure that the caller gets the error, but only if the caller is still reading from the channel. If the caller isn't reading from the channel, the request's goroutine will block trying to write to the channel and will never exit.

This patch solves this problem by returning.

  1. A channel of values, not results, that will be closed when the request is finished or canceled.
  2. An error channel with a buffer of 1 instead of a channel of results. This channel will yield at most one error before it's closed.

This pattern is used across a wide range of go projects including Docker. edit: docker doesn't consistently close the channels for some reason

func Request(ctx context.Context) (values <-chan ValueType, err <-chan error) {
    ch := make(chan ValueType)
    errCh := make(chan error, 1)
    go func() {
        defer close(errCh)
        defer close(ch)

        select {
        case ch<-value1:
        case <-ctx.Done():
            errCh<-ctx.Err()
        }
        select {
        case ch<-value2:
        case <-ctx.Done():
            errCh<-ctx.Err()
        }
    }()
    return ch, errCh
}

Unlike the previous API, this API is difficult to misuse.

  1. Errors are clearly returned on the err channel so the user won't assume that a closed value channel means the request succeeded.
  2. The value channel is closed so "for range" loops work.
  3. The error is always written to a channel with a buffer size of 1. If the caller doesn't listen on this channel (e.g., because it doesn't care about the error), nothing bad will happen.

The correct usage is:

values, err := Request(ctx)
for v := range values {
    // do something
}
// wait for the request to yield the final error
// (or close the channel with no error).
return <-err

The main downside to this approach is that it requires multiple channels.

Refactor the API to use error channels where appropriate.

Previously, asynchronous functions in this interface would return channels of
"results" that can be either errors or values as shown below

    type Result struct {
      Value ValueType
      Err error
    }
    func Request(context.Context) (results <-chan Result)

Unfortunately, there are several issues with this approach:

The basic problem with this approach is that it requires defining a new "result"
type and checking if each value is either a result or error. This is hardly a
show-stopper but it infects values with additional error fields.

However, the main issue with this approach is cancellation. If the context is
canceled, the Request implementation has a few options:

1. Close the results channel and walk away. Unfortunately, the caller will likely
   interpret this as "request competed" instead of "request canceled". To correctly
   use functions like this, the caller must check if the context after the channel
   closes.
2. Leave the result channel open. In this case, the caller must select on its
   own context. This is a pretty sane approach, but it makes it impossible to
   safely use the results channel in a "for range" loop.
3. Write the cancellation error to the results channel. This will ensure that
   the caller gets the error, but only if the caller is still reading from the
   channel. If the caller isn't reading from the channel, the request's
   goroutine will block trying to write to the channel and will never exit.

This patch solves this problem by returning an error channel with a buffer of 1
instead of a channel of results. This channel will yield at most one error
before it's closed. This pattern is used across a wide range of go projects
including Docker.

    func Request(context.Context) (values <-chan ValueType, err <-chan error)

Unlike the previous API, this API is difficult to misuse.

1. Errors are clearly returned on the err channel so the user won't assume that
   a closed value channel means the request succeeded.
2. The value channel is closed so "for range" loops work.
3. The error is always written to a channel with a buffer size of 1. If the
   caller doesn't listen on this channel (e.g., because it doesn't care about the
   error), nothing bad will happen.

The correct usage is:

    values, err := Request(ctx)
    for v := range values {
        // do something
    }
    // wait for the request to yield the final error
    // (or close the channel with no error).
    return <-err

The main downside to this approach is that it requires multiple channels.
@Stebalien Stebalien requested a review from magik6k May 6, 2020 03:53
@Stebalien
Copy link
Member Author

cc @MichaelMure if you wouldn't mind reviewing.

@Stebalien Stebalien requested a review from aschmahmann May 6, 2020 03:54
@Stebalien
Copy link
Member Author

For context: ipfs/kubo#4592 (comment)

var result []iface.Pin

for pin := range pins {
Copy link
Contributor

Choose a reason for hiding this comment

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

so, when an error happen, the result channel is not closed right ? Doesn't that means the caller would get stuck in this reading loop ?

Copy link
Member Author

Choose a reason for hiding this comment

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

With the new design, both the result and the error channels are always closed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm. I need to document that.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated the issue description and the interface documentation. It looks like docker isn't actually all that consistent.

@alanshaw
Copy link
Member

alanshaw commented May 6, 2020

SGTM - What about for DHT error types or refs errors - presumably they’re not the kind of error we’re talking about here and would still need the result type with the error property?

Copy link
Contributor

@aschmahmann aschmahmann left a comment

Choose a reason for hiding this comment

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

Seems like a reasonable plan to me. Left a few small comments.

Just want to double check if we have time to do this before we're supposed to be shipping 0.6.0 RC1.

Comment on lines -1 to -2
package iface

Copy link
Contributor

Choose a reason for hiding this comment

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

What's up with the changes to this file, is this just an unrelated cleanup?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yes. I couldn't find any users of this.

Comment on lines +23 to 25
// The returned channel will be closed when the request completes or is
// canceled.
FindProviders(context.Context, path.Path, ...options.DhtFindProvidersOption) (<-chan peer.AddrInfo, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why aren't we following the pattern here?

Copy link
Member Author

Choose a reason for hiding this comment

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

In this case, I'm asking IPFS to find me some providers for a key. If the request is canceled, we can just say "we're done" and walk away. That is, a partial response is still a valid response.

This is different from listing files/pins because:

  1. The request can be considered "complete" even if it returns a subset of the available providers. On the other hand, if Ls returned a subset of files/pins, that would be an incomplete result.
  2. Beyond "your arguments are invalid" and "this subsystem isn't ready (not bootstrapped)", there aren't really any useful errors the router could return other than "I can't find what you're looking for".

On the other hand, this brings up a good point. IMO, the Search function should have the same interface. That is NameAPI.Search(ctx, name, opts) (<-chan path.Path, error). Thoughts?

cc @alanshaw, I think this answers your question as well.

@lidel
Copy link
Member

lidel commented Jun 19, 2023

This repository is no longer maintained and has been copied over to boxo/coreiface and kubo/client/rpc.

You can learn more in the FAQs for the Boxo repo copying/consolidation effort.

In an effort to avoid noise and crippling in the Boxo repo from the weight of issues of the past, we are closing most issues and PRs in this repo. I've created ipfs/kubo#9974 to preserve and track this idea going forward.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants