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

context: Context.Done() is misleading, Cancelled() should be used instead #33523

Open
bickerx2 opened this issue Aug 7, 2019 · 3 comments

Comments

@bickerx2
Copy link

commented Aug 7, 2019

While learning Go I started working working with contexts I've found that the naming of the function that returns a Channel signaling if a context has been cancelled is misleading.

Take the following example:

package main

import (
        "context"
        "fmt"
        "time"
        "sync"
)

func fail(ctx context.Context, sleep int) {
        fmt.Println("(fail): I expect to be cancelled")
        select {
        case <-ctx.Done():
                fmt.Println("(fail): Something happen")
                return
        case <-time.After(time.Duration(sleep) * time.Second):
                fmt.Println("(fail): Something didn't happen")
                return
        }
}

func main() {
        fmt.Println("(main): A Go Context Demo")
        var blocker sync.WaitGroup
        ctx, cancel := context.WithCancel(context.Background())

        blocker.Add(1)
        go func() {
                defer blocker.Done()
                fail(ctx, 25)
        }()

        time.Sleep(4 * time.Second)
        fmt.Println("(main): Cancelling fail()")
        cancel()
        blocker.Wait()
        fmt.Println("(main): Goodbye")
}

Now have a look at this alternative, where I have replaced the Done() with Cancelled()

package main

import (
        "context"
        "fmt"
        "time"
        "sync"
)

func fail(ctx context.Context, sleep int) {
        fmt.Println("(fail): I expect to be cancelled")
        select {
        case <-ctx.Cancelled():
                fmt.Println("(fail): Something happen")
                return
        case <-time.After(time.Duration(sleep) * time.Second):
                fmt.Println("(fail): Something didn't happen")
                return
        }
}

func main() {
        fmt.Println("(main): A Go Context Demo")
        var blocker sync.WaitGroup
        ctx, cancel := context.WithCancel(context.Background())

        blocker.Add(1)
        go func() {
                defer blocker.Done()
                fail(ctx, 25)
        }()

        time.Sleep(4 * time.Second)
        fmt.Println("(main): Cancelling fail()")
        cancel()
        blocker.Wait()
        fmt.Println("(main): Goodbye")
}

I personally find the second version a lot more clear about what is going on as I no longer have to make the logic leap that when a context is "cancelled" that results in the channel that is returned via the to a function call of Done().

While it makes sense in the context of a context, as the context has been cancelled it's work has been done. This however is not as clear to people who have never used contexts before thus increasing their barrier of entry to the package (and ultimately the language as well)

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

Note that a context can also be "Done" if it times out, if it was created using context.WithTimeout or context.WithDeadline.

That said, you may well be right that Canceled would have been a better name than Done. But I don't see any reasonable way that we can change it now.

@dmitshur dmitshur changed the title context.Context.Done() is misleading, Cancelled() should be used instead context: Context.Done() is misleading, Cancelled() should be used instead Aug 7, 2019

@bickerx2

This comment has been minimized.

Copy link
Author

commented Aug 8, 2019

I would agree with what you saying, however a large amount documentation and existing guides I've encountered all talk about withTimeout ,withDeadline and withCancel all in the context of the Done() being called when the context has been cancelled.

Edit: Could please explain you can't see a reasonable way to change it when surely all that would be needed would be to rename Done() to Cancelled() and then re-create Done() as wrapper to Cancelled() which is deprecated?

@dmitshur dmitshur added this to the Go1.14 milestone Aug 12, 2019

@tv42

This comment has been minimized.

Copy link

commented Aug 13, 2019

@bickerx2 Context is an interface and if you add a new method to it you break all code that implements it independently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.