No direct klog calls, please. Use the logger instance instead.

What is the advantage of this custom code over runtime.HandleCrashWithContext?

Thanks for the suggestion! I used it in watch but not list. Here are my reasons:

1. runtime.HandleCrashWithContext by default re-panics after calling the handlers. That doesn't work here because the panic happens inside a goroutine in list(), and we need to pass the panic back to the main goroutine via panicCh. Re-panicking inside the child goroutine would bypass that and crash the process.

2. Setting ReallyCrash = false would avoid the re-panic, but it's a global variable and not thread-safe. Toggling it can trigger data races in tests or other concurrent code.

3. So instead, I used a local recover() block that mimics HandleCrashWithContext but without the re-panic. It still calls the registered panic handlers (so we get the logs and stack trace), and then sends the panic over the channel for the caller to handle.

4. If we think this pattern is useful more broadly, I’d be happy to propose a patch to staging/src/k8s.io/apimachinery/pkg/util/runtime/runtime.go to support it more cleanly.

nvm, just after I wrote these, I found I can make it cleaner by using

	defer func() {
		if r := recover(); r != nil {
			panicCh <- r
		}
	}()

	defer utilruntime.HandleCrashWithContext(ctx)

sends the panic over the channel for the caller to handle

Who needs this? What is the caller supposed to do with the panic?

We seem to have a policy in Kubernetes which is implemented by k8s.io/apimachinery/pkg/util/runtime: "panics should not occur and are fatal". If you feel that this code here needs a different policy, then I would like to see confirmation from SIG Arch on that.

I agree that for testing purposes it makes sense to apply a different policy, and I agree that the global variable in the runtime package is sub-optimal. If that is what you are trying to fix, then we could look for simpler alternatives. For example, a "don't exit" flag could be set in the context and get checked by the package first before falling back to the global variable.

Looking at this closer, I now understand why panicCh exists: list spawns a goroutine and any panic occurring there is meant to be seen as a panic in list itself.

Your latest incarnation doesn't achieve that. When calling utilruntime.HandleCrashWithContext(ctx) inside the goroutine, the entire process is getting torn down (default behavior outside of tests), without ever writing anything to panicCh.

Perhaps that is the right approach. But then the panicCh is redundant and should be removed.

To be clear, I'm in favor of this PR as far as it goes, plumbing ctx to the lister so we don't hang a background list. I just think we need to make our ctx adapter more correct if we're relying on it to interrupt the list for a context cancel OR remove the possibility of listerwatchers that don't have context (ideal, but more breaking)

(just a quick look in-tree shows ~9 places we're constructing cache.ListWatch with only ListFunc/WatchFunc methods, not the context equivalents ... looks like we haven't really finished the work to make lister watcher context aware ... so relying on it in this PR isn't super solid yet)

/hold

actually, it looks like we have multiple implementations of ListWithContext now that just drop the context:

// List a set of apiserver resources
func (lw *ListWatch) ListWithContext(ctx context.Context, options metav1.ListOptions) (runtime.Object, error) {
	// ListWatch is used in Reflector, which already supports pagination.
	// Don't paginate here to avoid duplication.
	if lw.ListWithContextFunc != nil {
		return lw.ListWithContextFunc(ctx, options)
	}
	return lw.ListFunc(options)
}

func (l listerWrapper) ListWithContext(ctx context.Context, options metav1.ListOptions) (runtime.Object, error) {
	return l.parent.List(options)
}

if we're relying on ctx cancellation here, we have to make those actually cancel on context cancel

We can fix all in-tree code, but ListWatch is a public API. There's always the possibility that some client-go consumer only supplies the non-context-aware functions. Perhaps we have to really remove those before we can proceed with this PR?

I think the minimum bar to rely on context cancellation here is:

1. converting all in-tree uses to honor context
2. making the places we adapt context-unaware listers to context-aware signatures unblock and return on context cancellation (leaving the uncancellable lists running/hanging in the background, I guess, with the panic propagation workaround shifted to those adapters)

To help move more of the out-of-tree ecosystem to threading context cancellation, we have a spectrum of options:

1. Sweep the internet for things that use our concrete ListWatch struct with ListFunc/WatchFunc and file issues or open PRs to switch them to provide ListWithContextFunc / WatchWithContextFunc instead. This is beneficial even if we never do any subsequent steps.
2. Possibly, after we've done 1 and waited some period of time, remove the non-context ListFunc/WatchFunc fields from ListWatch (while keeping the methods to satisfy the interface)
3. Possibly, after we've done 2 and waited some period of time, collapse the ListerWatcher and ListerWatcherWithContext interfaces.

we didn't log panics in this method before, I'm not really sure why this level is where we'd choose to start logging them now

I agree.

@ballista01: with "leave this to the caller" in #131000 (comment) I meant the caller of client-go, not a higher-level function in client-go. Please don't resolve comments without giving your reviewer a chance to check whether their comment really is addressed.