For defaults here I'd start with 10s (if no duration was passed in).
In the error condition we can drop that down to 2 seconds.

Okay, this is done, but also I should not that I refactored the default handling and stop timeout a little bit after realizing that waiting forever is the intended behavior when this function is called with seconds < 0. At least according to the comment on the ContainerStop function:

Lets set this explicitly to context.TODO(), especially since context should never be nil.

Then there is no need for var cancel, since that can be dealt with inside the if

TODO() because we really need to propagate a context down to this function, I have a separate PR that does that.
This will effectively allow an API request to cancel the wait instead of detaching from it.

okay will do

this is done, I have added the TODO context and made new contexts with timeouts internal to this func "subCtx"

I would bump this up to a higher value just because if the system is loaded this could have cause more issues.... maybe a minute?

I don't think we have the container lock here. Presuming I'm right waiting shouldn't cause any problems just so long as we eventually give up.

At this point the kill signal has failed for some reason, waiting for 1 minute seems overly long to me. Reason being that a caller of this function may set a timeout for how long they are willing to wait at a higher level than this function. If they passed in 10s as the timeout (the default) to container stop it seems a bit excessive to wait for an entire minute after the kill signal has failed.

If you'd really like to wait longer here, perhaps something on the order of 10 seconds?

@sparrc Currently the only way for the caller to set a timeout is from the passed in wait value. We could use that, but I think 2 seconds is too small and indefinite is too long.

In ECS we implemented a timeout to avoid our code waiting forever on docker returning from the ContainerStop function call. In my opinion this function shouldn't wait much longer than what the user passes in via the seconds argument.

If we use wait again here, then this function will usually take around wait seconds, with the possibility to take 2 * wait seconds. To me this is probably better than wait + 60s

I guess this is all a moot point anyways if you say that you have a change incoming to add the ability for the user to pass in a context anyways. I will amend this PR to use wait again here.

That being said, in the above code we lower wait to 2s when SIGTERM fails (ie after killPossiblyDeadProcess fails), so this logic may start to get complicated unless you would like me to also increase the wait after SIGTERM failure to something else?

We lost the comment explaining the reasoning for 2 seconds here. Can we add something back in? Otherwise this looks like a magic number without an explanation.

Also: this clobbers the value assigned to wait on line 48

it is a bit of a magic number, I can add a comment but Im not sure I have anything great to say about it. If you look around these files and kill functions there are quite a lot places where it does "this failed but wait 2 seconds to see if the container exited anyways", so I was just following that pattern.

Are you intentionally overwriting the value passed in as seconds though?

yes it's intentionally being overwritten, I'm not sure I agree with the behavior but the intention was to preserve the existing behavior as much as possible while getting rid of the infinite hang.

If you look at the deleted code in line 58 you can see that if killPossiblyDeadProcess returns an error then the container would be given 2 seconds to exit before calling killPossiblyDeadProcess(container, 9), so the intention here was just to maintain that behavior.

I guess instead of waiting just 2 seconds when killPossibleDeadProcess fails we could wait for the full seconds seconds.....I'm just not exactly sure what we'd be waiting for though since we already know that the kill function failed.

see comment below: #41586 (comment)

If seconds is 0, even if we set a value for wait on line 59 we're still not setting a timeout on the context? Is that intentional? Would it be better to check whether wait is zero here?

docker stop explicitly allows disabling the docker kill fallback by setting a negative timeout value (and thus waiting forever after SIGTERM). That's why it's checking >=.

If the user explicitly set the timeout for the wait after sending a stop signal to 0, then I guess we should respect that.

If the user explicitly set the timeout for the wait after sending a stop signal to 0, then I guess we should respect that.

You're doing that on line 59 irrespective of whether seconds was >= 0 which is why I'm confused.

see comment below: #41586 (comment)

If I'm summarizing that accurately, seconds < 0 == "user requested that we wait forever" (so we do so even when signal sending fails).

yep that's correct

I think it's worth inverting the conditional and out-denting the logic here since there's nothing left to do if there was no error

sure i can do that

I agree, but I also feel like there's an edge case here when the user specifies seconds < 0 (thus no timeout), but the "wait for container to stop" context gets cancelled for other reasons, such as the client disconnecting.

My interpretation of seconds < 0 is "client requested we wait forever" but that implies "client requested we 'stop' but never kill" so I think after the if err == nil block, we probably should also handle that edge case, right?

(Have to admit I'm a little bit out of my depth, so I might be misunderstanding the flow.)

Should we also explicitly invoke cancel() here since we know for sure that the current subCtx is finished? (before we confusingly reuse/shadow those variable names later in the "kill" block)

Doesn't calling context.WithCancel without ever calling cancel cause a minor leak?

https://golang.org/pkg/context/#WithCancel

Canceling this context releases resources associated with it, so code should call cancel as soon as the operations running in this Context complete.

So this should probably be something like this instead, right?

var subCtx context.Context, cancel context.CancelFunc
if seconds >= 0 {
	subCtx, cancel = context.WithTimeout(ctx, wait)
} else {
	subCtx, cancel = context.WithCancel(ctx)
}
defer cancel()

yes i can make this change

If I'm summarizing that accurately, seconds < 0 == "user requested that we wait forever" (so we do so even when signal sending fails).

I agree, but I also feel like there's an edge case here when the user specifies seconds < 0 (thus no timeout), but the "wait for container to stop" context gets cancelled for other reasons, such as the client disconnecting.

My interpretation of seconds < 0 is "client requested we wait forever" but that implies "client requested we 'stop' but never kill" so I think after the if err == nil block, we probably should also handle that edge case, right?

(Have to admit I'm a little bit out of my depth, so I might be misunderstanding the flow.)

Should we also explicitly invoke cancel() here since we know for sure that the current subCtx is finished? (before we confusingly reuse/shadow those variable names later in the "kill" block)