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

Jump to bottom

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

Closed
bickerx2 opened this issue Aug 7, 2019 · 4 comments
Closed

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

bickerx2 opened this issue Aug 7, 2019 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@bickerx2
Copy link

bickerx2 commented Aug 7, 2019 •
edited by ALTree

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
Copy link
Contributor

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
Copy link
Author

bickerx2 commented Aug 8, 2019 •
edited

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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 12, 2019
@dmitshur dmitshur added this to the Go1.14 milestone Aug 12, 2019
@tv42
Copy link

tv42 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.

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
@Sajmani
Copy link
Contributor

Sajmani commented Dec 16, 2020

Closing as infeasible. Per tv42's last comment, we cannot rename any methods of the Context interface while preserving the Go compatibility guarantee: https://golang.org/doc/go1compat

@Sajmani Sajmani closed this as completed Dec 16, 2020
@golang golang locked and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
7 participants
@rsc @tv42 @dmitshur @ianlancetaylor @gopherbot @Sajmani @bickerx2