Leaky go routines when using jwk.Cache

[sanchit-CRL]

Describe the bug
Initialising the jwk Cache creates multiple goroutines due to the underlying library https://github.com/lestrrat-go/httprc

Please attach the output of go version
go version go1.23.0 darwin/arm64

To Reproduce / Expected behavior

// GoroutineLeakChecker is a utility to check for goroutine leaks.
type GoroutineLeakChecker struct {
	initialGoroutines int
}

// NewGoroutineLeakChecker initializes a new GoroutineLeakChecker.
func NewGoroutineLeakChecker() *GoroutineLeakChecker {
	return &GoroutineLeakChecker{
		initialGoroutines: runtime.NumGoroutine(),
	}
}

// Check checks for goroutine leaks and returns an error if any are found.
func (glc *GoroutineLeakChecker) Check() error {
	time.Sleep(1 * time.Second) // Give some time for goroutines to settle
	finalGoroutines := runtime.NumGoroutine()
	if finalGoroutines > glc.initialGoroutines {
		return fmt.Errorf("goroutine leak detected: initial=%d, final=%d", glc.initialGoroutines, finalGoroutines)
	}
	return nil
}

func main() {
	glc := NewGoroutineLeakChecker()

	// initializing the cache creates leaky goroutines
	jwk.NewCache(context.Background())

	// Check for leaks
	if err := glc.Check(); err != nil {
		fmt.Println(err)
	} else {
		fmt.Println("No goroutine leaks detected")
	}
}

[lestrrat]

I think using SetGlobalFetcher did the job

• Provide a way to shutdown http cache fetcher to avoid go-routine leaks. #928

• jwx/jwk/jwk_test.go

  Lines 2166 to 2210 in 4c5a198

  	// This test is commented out because we did not want to include `go.uber.org/goleak`
  	// in our dependency. We agree that it's important to check for goroutine leaks,
  	// but 1) this is sort of expected in this library, and 2) we don't believe that
  	// forcing all of our users to use `go.uber.org/goleak` is prudent.
  	//
  	// However, we are leaving this test here so that users can learn how this function
  	// can be used. This is meant to show you the boilerplate code for when you want to make
  	// absolutely sure that no goroutine is left when you finish your program.
  	//
  	// For example, if you are writing an app, you can follow the pattern in the
  	// test below and stop the goroutines that are started by httprc.NewFetcher
  	// when your app terminates:
  	//
  	// func AppMain() {
  	// ctx, cancel := context.WithCancel(context.Background())
  	// defer cancel()
  	//
  	// jwk.SetGlobalFetcher(http.NewFetcher(ctx))
  	// // your app code goes here
  	// }
  	//
  	// Then, you can be sure that no goroutines are left when `AppMain()` is done.
  	// You can also verify this in your test:
  	//
  	// func TestApp(t *testing.T) {
  	// AppMain()
  	// goleak.VerifyNone(t)
  	// }
  	/*
  	func TestGH928(t *testing.T) {
  	ctx, cancel := context.WithCancel(context.Background())
  	jwk.SetGlobalFetcher(httprc.NewFetcher(ctx))
  	// If you are using a custom fetcher like this in your test and you
  	// still have more tests to run, you probably want to include the
  	// following line to restore the default behavior after this test.
  	// Otherwise, calls to `jwk.Fetch` may hang.
  	defer jwk.SetGlobalFetcher(nil) // This must be included to restore default behavior
  	cancel() // stop fetcher goroutines
  	// At this point, not goroutines from httprc.Fetcher should be running
  	goleak.VerifyNone(t)
  	}
  	*/

• jwx/jwk/fetch.go

  Lines 62 to 81 in 4c5a198

  	// SetGlobalFetcher allows users to specify a custom global fetcher,
  	// which is used by the `Fetch` function. Assigning `nil` forces
  	// the default fetcher to be (re)created when the next call to
  	// `jwk.Fetch` occurs
  	//
  	// You only need to call this function when you want to
  	// either change the fetching behavior (for example, you want to change
  	// how the default whitelist is handled), or when you want to control
  	// the lifetime of the global fetcher, for example for tests
  	// that require a clean shutdown.
  	//
  	// If you do use this function to set a custom fetcher, and you
  	// control its termination, make sure that you call `jwk.SetGlobalFetcher()`
  	// one more time (possibly with `nil`) to assign a valid fetcher.
  	// Otherwise, once the fetcher is invalidated, subsequent calls to `jwk.Fetch`
  	// may hang, causing very hard to debug problems.
  	//
  	// If you are sure you no longer need `jwk.Fetch` after terminating the
  	// fetcher, then you the above caution is not necessary.
  	func SetGlobalFetcher(f httprc.Fetcher) {

For most cases the above should just work.

If you encounter this when you are trying to do whatever it is that you want with the SetGlobalFetcher, you're going to have to bite the bullet and either live with the goroutines hanging around, or use v3.

[sanchit-CRL]

Thanks for the update, do we know when will v3 be released ?

[lestrrat]

@sanchit-CRL It's honestly mostly done, but I haven't found a compelling reason for users to migrate over to v3 (i.e., there's no single "killer" feature), and it's really just a collection of QoL improvements that also break API. I would love to move on to v3, but I also don't think anybody would at this point, so that's why it's been sitting in develop/v3 for some time now.

[github-actions]

This issue is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[github-actions]

This issue was closed because it has been stalled for 7 days with no activity. This does not mean your issue is rejected, but rather it is done to hide it from the view of the maintains for the time being. Feel free to reopen if you have new comments