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: the docs should better clarify the context timeout #70945

Closed
Muerte-here opened this issue Dec 20, 2024 · 10 comments · May be fixed by #71039
Closed

context: the docs should better clarify the context timeout #70945

Muerte-here opened this issue Dec 20, 2024 · 10 comments · May be fixed by #71039
Labels
Documentation Issues describing a change to documentation. help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@Muerte-here
Copy link

What is the URL of the page with the issue?

https://pkg.go.dev/[email protected]

What is your user agent?

Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/130.0.0.0 Safari/537.36

Screenshot

No response

What did you do?

Inspired by context package clarification made in #68923.

Since the context is one of the core package for golang concurrent programming, I believe that every segment of its behavior should be clearly written and documented.

In many places, the context package documentation makes a "difference" (as it should IMO) between context cancellation (done by calling the cancel function) and context timeout (deadline passed). For example:

  1. two error variables and their descriptions:
  • Canceled is the error returned by [Context.Err] when the context is canceled.
  • DeadlineExceeded is the error returned by [Context.Err] when the context's deadline passes.
  1. the description of AfterFunc uses the term "done" to denote both situations:

AfterFunc arranges to call f in its own goroutine after ctx is done (canceled or timed out).

  1. the description of Err() error also makes difference

// If Done is closed, Err returns a non-nil error explaining why:
// Canceled if the context was canceled
// or DeadlineExceeded if the context's deadline passed.

etc...

However, in some places, the documentation remains unclear about the time-out.

For example:

  1. function Cause:

Cause returns a non-nil error explaining why c was canceled. The first cancellation of c or one of its parents sets the cause. If that cancellation happened via a call to CancelCauseFunc(err), then Cause returns err. Otherwise Cause(c) returns the same value as c.Err(). Cause returns nil if c has not been canceled yet.

From the last sentence to the first.

It says that Cause returns nil if c has not yet been canceled. It is not true. It returns a non-nil value when the context has not yet been canceled (the cancel function has not been called) but deadline has expired (its or one of its parents). IMO the same term used by AfterFunc - done - can be used here as well. Thus, "Cause returns nil if c is not done yet (canceled or timed out; itself or one of its parent)".

Next three sentences explain only the behavior when the context is cancelled, but not when the c's or one its parents deadline has passed. It may seem that it is covered under "Otherwise, ..." but to me it looks more like it covers the case when CancelFunc is called instead of CancelCauseFunc.

The first sentence is incomplete. Cause returns a non-nil error not only when c is canceled but also when c or any of its parent contexts times out. Again, IMO the term done used in the AfterFunc fits well here. Thus, "Cause returns a non-nil error explaining why c is done (canceled or timed out; itself or one of its parent)."

  1. type CancelCauseFunc:

If the context has already been canceled, CancelCauseFunc does not set the cause.

This seems incomplete to me. It also does not set the cause in the case when the deadline of the context or one of its parents has passed. IMO the term done can be used here again. Further, the examples should also be corrected.

  1. function AfterFunc:

AfterFunc arranges to call f in its own goroutine after ctx is done (canceled or timed out). If ctx is already done, AfterFunc calls f immediately in its own goroutine.

Everything looks great here, but I believe it should be mentioned that this also applies when one of the parent contexts times out. Nowhere in the specification does it state that if a context times out, all its derived children also time out, which makes the description of AfterFunc feel somewhat incomplete. IMO, we just need to add the note about parents that I mentioned earlier for done in the other examples.

  1. Overview:

There is only a note that if a Context is canceled, all contexts derived from it are also canceled.

When a Context is canceled, all Contexts derived from it are also canceled.

IMO, it should also be noted that if a Context times out, all Contexts derived from it also time out (even if they don't have their own timer, such as those created with WithCancel). For instance, in the following example ctxChild (even though created without timer) will timeout together with its parent:

package main

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

func main() {
	ctxParent, _ := context.WithDeadline(context.TODO(), time.Now().Add(time.Millisecond))
	ctxChild, _ := context.WithCancel(ctxParent) // context without "timer"

	time.Sleep(time.Second)

	ch := make(chan struct{})

	context.AfterFunc(ctxChild, func() {
		fmt.Println("hi")
		ch <- struct{}{}
	})

	<-ch
}

In this way, there would be no need to add notes about parents within the term done, thus it will be as it currently appears in AfterFunc.

What did you see happen?

Insufficiently explained behavior in the case of a timeout.

What did you expect to see?

Adding a note in the overview about the relationship between a context and its child in the case of a timeout, and the usage of the term done.

@gopherbot gopherbot added the Documentation Issues describing a change to documentation. label Dec 20, 2024
@ianlancetaylor ianlancetaylor added help wanted NeedsFix The path to resolution is known, but the work has not been done. and removed pkgsite labels Dec 21, 2024
@ianlancetaylor ianlancetaylor added this to the Backlog milestone Dec 21, 2024
@ianlancetaylor
Copy link
Member

Thanks. Would you like to send a patch improving the documentation? See https://go.dev/doc/contribute.

@Muerte-here
Copy link
Author

@ianlancetaylor Hi, Ian.

Of course, I'd be glad to. However, I currently don’t have enough time to dedicate myself fully to it. Beyond what I’ve mentioned in the issue, I’m unsure if there are other omissions in the documentation regarding timeouts, and I’d prefer to address everything comprehensively. I would greatly appreciate it if you or someone else could take this on in the meantime. That said, if no one steps in, I will certainly dedicate myself to it.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/638975 mentions this issue: context: the docs should better clarify the context timeout

@glycerine
Copy link

  1. It would be simpler, and thus less verbose, to simply define the deadline passing as also meaning cancelled. To my understanding:

There are multiple ways a context can be cancelled. The deadline can pass, the timeout can occur, or the user can call the cancelFunc. They all mean the same thing: that the context has been cancelled.

  1. When I read the suggestion,

// When a Context is canceled or its deadline passes,
// all Contexts derived from it are also considered done (canceled or timed out, respectively).
// This applies to derived Contexts regardless of whether they were created with their own deadline or not.

I think this is not strictly true; it is not true for derived Contexts that have had WithoutCancel() called on them.

@Muerte-here
Copy link
Author

@glycerine

There are multiple ways a context can be cancelled. The deadline can pass, the timeout can occur, or the user can call the cancelFunc. They all mean the same thing: that the context has been cancelled.

The current documentation and the code differentiate between cancellations and timeouts. Additionally, I'm not entirely sure I understand the distinction you made between "the deadline can pass" and "the timeout can occur"—they are the same thing.

I think this is not strictly true; it is not true for derived Contexts that have had WithoutCancel() called on them.

Yes, that's an exception and the doc for WithoutCancel() clearly states it (just not canceled should be replaced with not done IMO):

WithoutCancel returns a derived context that points to the parent context and is not canceled when parent is canceled.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/640095 mentions this issue: context: use "canceled" in docs to refer to timed-out contexts

@neild
Copy link
Contributor

neild commented Jan 3, 2025

It would be simpler, and thus less verbose, to simply define the deadline passing as also meaning cancelled.

I agree.

In practice, if you see a reference to a "canceled context" in documentation, that almost certainly is intended to cover contexts past their deadline. We should just document that a context is canceled when its deadline passes.

@dmitshur dmitshur modified the milestones: Backlog, Go1.24 Jan 3, 2025
@Muerte-here
Copy link
Author

Muerte-here commented Jan 4, 2025

@neild This is a bad decision IMO. There definitely should have been a difference between cancellation and timeout. The first is the result of a cancel function call, while the second is the result of a timeout. That's why there are two errors that reflect those two situations. The grouping of both situations should have been marked with "done" (as is the name of the function inside the interface).

With the current change, when it is said that the context was canceled, it will not be known in what way it was canceled.

With my proposal, when we say that the context is canceled, then it is known exactly that the cancel function was called (directly or through the parent function), similarly for timeout. "Done" would mean that the channel was closed in one of those two ways.

@ianlancetaylor @neild Please consider reopening this issue.

@ianlancetaylor
Copy link
Member

Although it's true that Done is the name of the Context method, in my opinion "done" doesn't read well in the comments. I think "canceled" is clearer, with the understanding that the Context may be canceled either by calling a cancellation function or by timing out.

Note in particular that the Done method does not report whether the context is done. It returns a channel that's "closed when work done on behalf of this context should be canceled." So the terms are ambiguous all around.

I think the current comment text does clarify this issue.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Issues describing a change to documentation. help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants