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

proposal: context.WithSignal #21521

Closed
markbates opened this Issue Aug 18, 2017 · 21 comments

Comments

Projects
None yet
@markbates

markbates commented Aug 18, 2017

A common design pattern when building applications is to trap signals, such as CTRL-C, and gracefully close down the application.

One might think this pattern is fairly straightforward and easy to implement. To illustrate the difficulties in getting this pattern correct, let's look at the following example.

In June, @matryer, wrote a blog post demonstrating how to capture signals in your application and use them to gracefully shutdown an application. This post resulted in a fantastic thread on Twitter with @Sajmani and @quentinmit pointing out the mistakes in Mat's code that demonstrate the subtleties in getting the implementation correct .

I propose that this type of cancellation, by signals, should be a part of the context package, as it follows the same idea as cancellation after a certain time, or by a deadline. Not only does this solve the problems with capturing signals that most people make, but, I believe, is another great use case for cancellation via contexts.

An example of this addition to the context package would work is as follows:

func main() {
	ctx, cancel := context.WithSignal(context.Background(), os.Interrupt)
	defer cancel()
	select {
	case <-ctx.Done():
		fmt.Println("thanks for stopping me")
	}
}

And a proposed implementation of the WithSignal function.

func WithSignal(ctx Context, s ...os.Signal) (Context, CancelFunc) {
	ctx, cancel := WithCancel(ctx)
	c := make(chan os.Signal, 1)
	signal.Notify(c, s...)
	go func() {
		select {
		case <-c:
			cancel()
		case <-ctx.Done():
			cancel()
		}
		signal.Stop(c)
	}()
	return ctx, cancel
}

@gopherbot gopherbot added this to the Proposal milestone Aug 18, 2017

@gopherbot gopherbot added the Proposal label Aug 18, 2017

@OneOfOne

This comment has been minimized.

Show comment
Hide comment
@OneOfOne

OneOfOne Aug 18, 2017

Contributor

I'd rather have that in the signal package.

Contributor

OneOfOne commented Aug 18, 2017

I'd rather have that in the signal package.

@davecheney

This comment has been minimized.

Show comment
Hide comment
@davecheney

davecheney Aug 19, 2017

Contributor
Contributor

davecheney commented Aug 19, 2017

@matryer

This comment has been minimized.

Show comment
Hide comment
@matryer

matryer Aug 19, 2017

matryer commented Aug 19, 2017

@matryer

This comment has been minimized.

Show comment
Hide comment
@matryer

matryer Aug 19, 2017

matryer commented Aug 19, 2017

@Sajmani

This comment has been minimized.

Show comment
Hide comment
@Sajmani

Sajmani Aug 20, 2017

Contributor
Contributor

Sajmani commented Aug 20, 2017

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Aug 21, 2017

Contributor

To echo Dave Cheney's point: Context is request scoped but signals are process scoped.

I'm not sure this proposal makes sense at all.

Contributor

rsc commented Aug 21, 2017

To echo Dave Cheney's point: Context is request scoped but signals are process scoped.

I'm not sure this proposal makes sense at all.

@bradleyfalzon

This comment has been minimized.

Show comment
Hide comment
@bradleyfalzon

bradleyfalzon Aug 22, 2017

I'm not sure this proposal makes sense at all.

My use case, and I assume others, is that I have a program that starts a series of goroutines that need cancellation, it also calls functions which accept contexts that allow me to terminate them.

All of these operations are passed a context in order to cancel them, having the context derived from a single context created at application start with a cancel function similar^ to what's described by the OP.

When my program needs to exit, the process manager sends a signal, which is caught by the main package and the top level context is cancelled, propogating this to the goroutines and stopping any long running functions (http requests or commands). WaitGroups then wait until everything has finished successfully and then the program terminates.

This is why I don't understand the comment Context is request scoped but signals are process scoped. My application started a series of routines, is using context as I've described and implemented it like the OP has proposed an incorrect way to safely shutdown?

^ The OP's proposal is correct, my implementation is not, so I'm in favour of adding this to signal package or having a copy/paste example documented.

bradleyfalzon commented Aug 22, 2017

I'm not sure this proposal makes sense at all.

My use case, and I assume others, is that I have a program that starts a series of goroutines that need cancellation, it also calls functions which accept contexts that allow me to terminate them.

All of these operations are passed a context in order to cancel them, having the context derived from a single context created at application start with a cancel function similar^ to what's described by the OP.

When my program needs to exit, the process manager sends a signal, which is caught by the main package and the top level context is cancelled, propogating this to the goroutines and stopping any long running functions (http requests or commands). WaitGroups then wait until everything has finished successfully and then the program terminates.

This is why I don't understand the comment Context is request scoped but signals are process scoped. My application started a series of routines, is using context as I've described and implemented it like the OP has proposed an incorrect way to safely shutdown?

^ The OP's proposal is correct, my implementation is not, so I'm in favour of adding this to signal package or having a copy/paste example documented.

@matryer

This comment has been minimized.

Show comment
Hide comment
@matryer

matryer Aug 22, 2017

matryer commented Aug 22, 2017

@mattn

This comment has been minimized.

Show comment
Hide comment
@mattn

mattn Aug 22, 2017

Member

If several packages handle signals for each packages, and using them, it won't work expectedly. This proposal may provide wrong usages to the developers of third-packages.

Member

mattn commented Aug 22, 2017

If several packages handle signals for each packages, and using them, it won't work expectedly. This proposal may provide wrong usages to the developers of third-packages.

@matryer

This comment has been minimized.

Show comment
Hide comment
@matryer

matryer Aug 22, 2017

matryer commented Aug 22, 2017

@jimmyfrasche

This comment has been minimized.

Show comment
Hide comment
@jimmyfrasche

jimmyfrasche Aug 22, 2017

Member

Context is request scoped but signals are process scoped.

The process is a request from the user to the OS.

For example, if I have a cronjob that makes network requests that result in DB transactions, I want to be able to kill it gracefully (and, perhaps more importantly in this example, to set a process-wide deadline if it cannot finish in a reasonable time so it doesn't spin all night).

Having the process be the root of the context tree makes a lot of sense in that situation. It may not always make sense, certainly, but there's nothing unreasonable about this.

I am not convinced that it belongs in stdlib. An example in the docs, a wiki page, a blog post explaining the subtleties involved, or a third-party library all seem reasonable, though.

Member

jimmyfrasche commented Aug 22, 2017

Context is request scoped but signals are process scoped.

The process is a request from the user to the OS.

For example, if I have a cronjob that makes network requests that result in DB transactions, I want to be able to kill it gracefully (and, perhaps more importantly in this example, to set a process-wide deadline if it cannot finish in a reasonable time so it doesn't spin all night).

Having the process be the root of the context tree makes a lot of sense in that situation. It may not always make sense, certainly, but there's nothing unreasonable about this.

I am not convinced that it belongs in stdlib. An example in the docs, a wiki page, a blog post explaining the subtleties involved, or a third-party library all seem reasonable, though.

@markbates

This comment has been minimized.

Show comment
Hide comment
@markbates

markbates Aug 22, 2017

I think having this in the signal package or in an x package would probably make more sense than having it in the context package.

If anything, I think this issue shines a light on the messaging problem around "context". Should it be used for cancellation? Should it be used for value passing? What is a "request"?

If the pattern of capturing signals and canceling a context from within the main function is a Go anti-pattern, then that needs to be communicated, because that is what is happening in the wild.

markbates commented Aug 22, 2017

I think having this in the signal package or in an x package would probably make more sense than having it in the context package.

If anything, I think this issue shines a light on the messaging problem around "context". Should it be used for cancellation? Should it be used for value passing? What is a "request"?

If the pattern of capturing signals and canceling a context from within the main function is a Go anti-pattern, then that needs to be communicated, because that is what is happening in the wild.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Aug 28, 2017

Contributor

All of these operations are passed a context in order to cancel them, having the context derived from a single context created at application start with a cancel function similar^ to what's described by the OP.

I'm not convinced we want to encourage tying contexts to signals in this way, as described above. I appreciate that the code was difficult to get right but all of it was about interacting with the os/signal package, not context. It sounds like maybe we haven't gotten the os/signal API right, or at least easy enough to use. (And that's entirely believable because signals are asynchronous events and those are just hard in general.)

In Mat's post:

func main() {
	ctx := context.Background()

	// trap Ctrl+C and call cancel on the context
	ctx, cancel := context.WithCancel(ctx)
	c := make(chan os.Signal, 1)
	signal.Notify(c, os.Interrupt)
	defer func() {
		signal.Stop(c)
		cancel()
	}()
	go func() {
		select {
		case <-c:
			cancel()
		case <-ctx.Done():
		}
	}()
	
	doSomethingAwesome(ctx)
}

The part using context is really just one line: ctx, cancel := context.WithCancel(ctx). The hard part is hooking up that cancel function to os/signal. I would focus on what we can do to make that wiring easier - because probably it's too hard to wire to things other than context as well - instead of just solving this problem once inside context but not more generally.

It seems like there is a general problem here that we should solve instead of solving the very specific (and arguably not too useful) one of triggering a context cancellation when an incoming signal arrives.

Contributor

rsc commented Aug 28, 2017

All of these operations are passed a context in order to cancel them, having the context derived from a single context created at application start with a cancel function similar^ to what's described by the OP.

I'm not convinced we want to encourage tying contexts to signals in this way, as described above. I appreciate that the code was difficult to get right but all of it was about interacting with the os/signal package, not context. It sounds like maybe we haven't gotten the os/signal API right, or at least easy enough to use. (And that's entirely believable because signals are asynchronous events and those are just hard in general.)

In Mat's post:

func main() {
	ctx := context.Background()

	// trap Ctrl+C and call cancel on the context
	ctx, cancel := context.WithCancel(ctx)
	c := make(chan os.Signal, 1)
	signal.Notify(c, os.Interrupt)
	defer func() {
		signal.Stop(c)
		cancel()
	}()
	go func() {
		select {
		case <-c:
			cancel()
		case <-ctx.Done():
		}
	}()
	
	doSomethingAwesome(ctx)
}

The part using context is really just one line: ctx, cancel := context.WithCancel(ctx). The hard part is hooking up that cancel function to os/signal. I would focus on what we can do to make that wiring easier - because probably it's too hard to wire to things other than context as well - instead of just solving this problem once inside context but not more generally.

It seems like there is a general problem here that we should solve instead of solving the very specific (and arguably not too useful) one of triggering a context cancellation when an incoming signal arrives.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Aug 28, 2017

Contributor

Also, in func main the code in https://twitter.com/matryer/status/869096368039710720 is completely fine and maybe should have been left as it was. :-)

Contributor

rsc commented Aug 28, 2017

Also, in func main the code in https://twitter.com/matryer/status/869096368039710720 is completely fine and maybe should have been left as it was. :-)

@AlekSi

This comment has been minimized.

Show comment
Hide comment
@AlekSi

AlekSi Aug 30, 2017

Contributor

I would prefer to have an example of correct usage in documentation instead of a new method.

Contributor

AlekSi commented Aug 30, 2017

I would prefer to have an example of correct usage in documentation instead of a new method.

@kamilsk

This comment has been minimized.

Show comment
Hide comment
@kamilsk

kamilsk Aug 30, 2017

What happens when many calls to WithSignal occur in the same context tree?
This feels weird. Context is request scoped but signals are process scoped.

Good point for me.

Maybe is it better to hang the logic outside the context package? I think it is a more flexible way.

As an example what I use:
https://github.com/kamilsk/semaphore/blob/5c82a8b62d3b3009be756f8f9afaea94b3a89961/context.go#L8-L15
https://github.com/kamilsk/semaphore/blob/5c82a8b62d3b3009be756f8f9afaea94b3a89961/channel.go#L35-L49
I removed dependency to "context" but saved "cancellation" feature

For instance:

middleware := func(handler http.Handler) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		ctx := WithInterrupter(r.Context(), Combine(
			WithSignal(os.Interrupt),
			WithEvent(event.UserDeleted, event.UserBanned),
			WithTimeout(service.SLA),
			WithDeadline(startup.Ruin),
		))
		handler.ServeHTTP(w, r.WithContext(ctx))
	})
}

Call the WithTimeout (or WithDeadline) several times does not look strange:

global := func(critical time.Duration, handler http.Handler) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		ctx, cancel := context.WithTimeout(r.Context(), critical)
		defer cancel()
		
		handler.ServeHTTP(w, r.WithContext(ctx))
	})
}

search := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
	// config.SearchTimeout < critical, maybe config.SearchTimeout << critical
	ctx, cancel := context.WithTimeout(r.Context(), config.SearchTimeout)
	defer cancel()
	
	result := doSearch(ctx)
	// write response
})

http.Handle("/search", global(config.SLA, search))

But what about WithSignal?

kamilsk commented Aug 30, 2017

What happens when many calls to WithSignal occur in the same context tree?
This feels weird. Context is request scoped but signals are process scoped.

Good point for me.

Maybe is it better to hang the logic outside the context package? I think it is a more flexible way.

As an example what I use:
https://github.com/kamilsk/semaphore/blob/5c82a8b62d3b3009be756f8f9afaea94b3a89961/context.go#L8-L15
https://github.com/kamilsk/semaphore/blob/5c82a8b62d3b3009be756f8f9afaea94b3a89961/channel.go#L35-L49
I removed dependency to "context" but saved "cancellation" feature

For instance:

middleware := func(handler http.Handler) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		ctx := WithInterrupter(r.Context(), Combine(
			WithSignal(os.Interrupt),
			WithEvent(event.UserDeleted, event.UserBanned),
			WithTimeout(service.SLA),
			WithDeadline(startup.Ruin),
		))
		handler.ServeHTTP(w, r.WithContext(ctx))
	})
}

Call the WithTimeout (or WithDeadline) several times does not look strange:

global := func(critical time.Duration, handler http.Handler) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		ctx, cancel := context.WithTimeout(r.Context(), critical)
		defer cancel()
		
		handler.ServeHTTP(w, r.WithContext(ctx))
	})
}

search := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
	// config.SearchTimeout < critical, maybe config.SearchTimeout << critical
	ctx, cancel := context.WithTimeout(r.Context(), config.SearchTimeout)
	defer cancel()
	
	result := doSearch(ctx)
	// write response
})

http.Handle("/search", global(config.SLA, search))

But what about WithSignal?

@DeedleFake

This comment has been minimized.

Show comment
Hide comment
@DeedleFake

DeedleFake Sep 5, 2017

This is technically a duplicate of #16472, isn't it? Not saying it can't be reevaluated, though.

DeedleFake commented Sep 5, 2017

This is technically a duplicate of #16472, isn't it? Not saying it can't be reevaluated, though.

@matryer

This comment has been minimized.

Show comment
Hide comment
@matryer

matryer Sep 5, 2017

matryer commented Sep 5, 2017

@bcmills

This comment has been minimized.

Show comment
Hide comment
@bcmills

bcmills Sep 7, 2017

Member

It seems like there is a general problem here that we should solve instead of solving the very specific (and arguably not too useful) one of triggering a context cancellation when an incoming signal arrives.

I believe that the general problem here is closely related to the one in #16620: function calls and channel operations are closely related, but it is often difficult, tedious, or inefficient to convert between the two.

You could imagine a generic (#15292) API for converting channel operations to function calls.

package chanfunc

func Receive(c <-T, f func(T)) {
	for v := range c {
		f(v)
	}
}

Then the boilerplate here would look like:

func main() {
	// trap Ctrl+C and call cancel on the context
	ctx, cancel := context.WithCancel(context.Background())
	c := make(chan os.Signal, 1)
	signal.Notify(c, os.Interrupt)
	chanfunc.Receive(c, func(os.Signal) { cancel() })
	
	doSomethingAwesome(ctx)
}

A non-generic version of that would be to build the composition of signal.Notify and chanfunc.Receive into the Signal package as a single function:

package signal

func NotifyFunc(func(os.Signal), os.Signal...)
package main

func main() {
	// trap Ctrl+C and call cancel on the context
	ctx, cancel := context.WithCancel(context.Background())
	signal.NotifyFunc(func(os.Signal) { cancel() }, os.Interrupt)
	
	doSomethingAwesome(ctx)
}
Member

bcmills commented Sep 7, 2017

It seems like there is a general problem here that we should solve instead of solving the very specific (and arguably not too useful) one of triggering a context cancellation when an incoming signal arrives.

I believe that the general problem here is closely related to the one in #16620: function calls and channel operations are closely related, but it is often difficult, tedious, or inefficient to convert between the two.

You could imagine a generic (#15292) API for converting channel operations to function calls.

package chanfunc

func Receive(c <-T, f func(T)) {
	for v := range c {
		f(v)
	}
}

Then the boilerplate here would look like:

func main() {
	// trap Ctrl+C and call cancel on the context
	ctx, cancel := context.WithCancel(context.Background())
	c := make(chan os.Signal, 1)
	signal.Notify(c, os.Interrupt)
	chanfunc.Receive(c, func(os.Signal) { cancel() })
	
	doSomethingAwesome(ctx)
}

A non-generic version of that would be to build the composition of signal.Notify and chanfunc.Receive into the Signal package as a single function:

package signal

func NotifyFunc(func(os.Signal), os.Signal...)
package main

func main() {
	// trap Ctrl+C and call cancel on the context
	ctx, cancel := context.WithCancel(context.Background())
	signal.NotifyFunc(func(os.Signal) { cancel() }, os.Interrupt)
	
	doSomethingAwesome(ctx)
}
@rv-jhester

This comment has been minimized.

Show comment
Hide comment
@rv-jhester

rv-jhester Sep 26, 2017

This may not be the best solution, but this is definitely a pain point, and is definitely worth further discussion.

rv-jhester commented Sep 26, 2017

This may not be the best solution, but this is definitely a pain point, and is definitely worth further discussion.

@rsc

This comment has been minimized.

Show comment
Hide comment
@rsc

rsc Oct 16, 2017

Contributor

I wrote above:

The part using context is really just one line: ctx, cancel := context.WithCancel(ctx). The hard part is hooking up that cancel function to os/signal. I would focus on what we can do to make that wiring easier - because probably it's too hard to wire to things other than context as well - instead of just solving this problem once inside context but not more generally.

It seems like there is a general problem here that we should solve instead of solving the very specific (and arguably not too useful) one of triggering a context cancellation when an incoming signal arrives.

I think we should decline this proposal and leave open the question of addressing os/signal more generally. If anyone wants to make specific concrete proposals about the latter in new issues, please do.

Contributor

rsc commented Oct 16, 2017

I wrote above:

The part using context is really just one line: ctx, cancel := context.WithCancel(ctx). The hard part is hooking up that cancel function to os/signal. I would focus on what we can do to make that wiring easier - because probably it's too hard to wire to things other than context as well - instead of just solving this problem once inside context but not more generally.

It seems like there is a general problem here that we should solve instead of solving the very specific (and arguably not too useful) one of triggering a context cancellation when an incoming signal arrives.

I think we should decline this proposal and leave open the question of addressing os/signal more generally. If anyone wants to make specific concrete proposals about the latter in new issues, please do.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.