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: AfterFunc spawns a goroutine #61672

Closed
akuzmin-google opened this issue Jul 31, 2023 · 6 comments
Closed

context: AfterFunc spawns a goroutine #61672

akuzmin-google opened this issue Jul 31, 2023 · 6 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@akuzmin-google
Copy link

What version of Go are you using (go version)?

1.21

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

N/A

What did you do?

We are running a Stubby RPC server for a service processing ~10'000 in-flight RPCs. This server has performance issues where locks get held for excessive durations. Thanks to the help of @aktau, we were able to attribute some of this to the Go scheduler having trouble with the ~100'000 goroutines our server ends up running.

A lof of these were caused by goroutines waiting for channels, either in context.propagateCancel, or in other cancellation propagation mechanisms (e.g. Stubby connection breaking cancelling the RPC).

Thus, we were expecting to be able to use AfterFunc in some of the libraries to reduce the total number of goroutines.

What did you expect to see?

Based on #57928 mentioning performance and avoiding goroutines as motivating reasons, I expected this to not spawn a separate goroutine, but rather use some kind of callback registration. So I expected fewer goroutines in total on our server.

What did you see instead?

Reading the code, context.AfterFunc ends up using the propagateCancel API internally, which spawns a goroutine to wait on cancellation. Thus, using context.AfterFunc does not reduce the goroutines.

It looks like there is an afterFuncer interface defined to do exactly this, but it is not implemented by any of the stdlib contexts. It is also not exported (though implementing our own context and rolling it out everywhere would not be viable either way).

Filing this issue on the public Go repo on the suggestion of @aktau, please let me know if I am missing any information as this is the first time I do this!

@neild
Copy link
Contributor

neild commented Jul 31, 2023

AfterFunc(ctx, f) will start a goroutine if all of the following are true:

  • ctx was not created by the context package.
  • ctx.Done() is not nil.
  • ctx is not already done (that is, ctx.Done() is not closed).
  • ctx does not have an AfterFunc(func()) func() bool method.

In this case, ctx is a third-party implementation of the Context interface, and there is no way to tell when ctx has been canceled except by reading from its done channel.

Stubby is a Google-internal RPC system which uses its own Context implementation (because it predates the context package, and was in fact the inspiration for that package). The fix here will be to add an AfterFunc method to the Stubby Context implementation. (Or to change Stubby to use standard library contexts, but that'd probably be more work.)

@akuzmin-google
Copy link
Author

Could you help me understand where AfterFunc is defined on the standard context? I can't seem to find it looking at the code, but it's possible I am missing something really obvious. I tried finding anything implementing afterFuncer and failed.

I see the static func AfterFunc method, but I don't see a func (ctx context.Context) AfterFunc or equivalent?

@neild
Copy link
Contributor

neild commented Jul 31, 2023

The context package doesn't declare an AfterFunc method on the contexts it creates.

For some historical context: The context.Context type is an interface, and we originally imagined that there might be many implementations of this interface. However, supporting multiple implementations turned out to be of little practical benefit, and makes it difficult to efficiently implement various context operations. Notably, prior to the introduction of AfterFunc in Go 1.21, there is no way to propagate cancelation from one arbitrary context to another without starting a background goroutine. For this reason, the Google-internal style guide has long forbidden creating custom context implementations. If we could go back and do it over again, we'd probably define context.Context as a concrete type rather than an interface.

The context package has optimized paths for working with its own contexts. Notably, cancelCtx.propagateCancel checks to see if the parent context is a *context.cancelCtx. If it is, it registers the child context with the parent for cancelation:
https://go.googlesource.com/go/+/refs/tags/go1.21rc3/src/context/context.go#473

As of Go 1.21, the context package supports a more efficient mechanism for propagating cancelation from a third-party context, via an AfterFunc method:
https://go.googlesource.com/go/+/refs/tags/go1.21rc3/src/context/context.go#489

The context package doesn't implement AfterFunc methods on its own contexts, because it has that preexisting fast path for them. However, it can avoid starting background goroutines when working with third-party contexts that implement AfterFunc.

@dr2chase
Copy link
Contributor

dr2chase commented Aug 3, 2023

Is this WAI and workaround supplied? (It looks like that to me).

@neild
Copy link
Contributor

neild commented Aug 3, 2023

I believe so, yes.

@neild neild closed this as not planned Won't fix, can't repro, duplicate, stale Aug 3, 2023
@dmitshur dmitshur added this to the Go1.21 milestone Aug 3, 2023
@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 3, 2023
@akuzmin-google
Copy link
Author

Yes, meant to close this. To confirm, that answers my question!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants