Improper usage of keyfunc leading to leaking goroutines and compounding scheduled HTTP requests

[MicahParks]

Currently there are two functions that use the github.com/MicahParks/keyfunc package.

1. Apple related function
2. Google Workspaces related function

They both share this snippet of code:

// Create the keyfunc options. Refresh the JWKS every hour and log errors.
refreshInterval := time.Hour
options := keyfunc.Options{
  RefreshInterval: refreshInterval,
}

// Create the JWKS from the resource at the given URL.
jwks, err := keyfunc.Get(jwksURL, options)
if err != nil {
  return claims, err
}

// Parse the JWT.
token, err := jwt.ParseWithClaims(idToken, claims, jwks.Keyfunc)
if err != nil {
  return claims, err
}

When using the RefreshInterval field in the keyfunc.Options data structure, a background goroutine will be launched to refresh the JWKS at the given interval. Additionally, there is a comment that suggests errors from the keyfunc's background goroutine will be logged, however, the RefreshErrorHandler field was not specified, so this is not the case. Reference.

Since the function creates a new *keyfunc.JWKS via keyfunc.Get every time it is called and it does not end the background goroutine for the *keyfunc.JWKS, these two functions leak goroutines every time they are called. This can be problematic because it is a resource leak and each of these leaked goroutines will make an HTTP request to their JWKS endpoints at the given interval, forever.

As a note, a *keyfunc.JWKS's background goroutine can be ended via the EndBackground method or via the context given by the keyfunc.Options to the keyfunc.Get function.

I suggest doing one of the two things:

1. Pass an empty keyfunc.Options to the keyfunc.Get function. This will keep the current behavior, but no background goroutine will be launched.
2. Move the creation of the *keyfunc.JWKS outside of these functions. (The creation of this data structure is the result of a call to the keyfunc.Get function.) This would allow the same *keyfunc.JWKS to be reused across function calls. If you chose this option, I would also suggest exposing a way for the users of the supertokens-golang package to end any background goroutines.

[rishabhpoddar]

Thanks for raising this issue @MicahParks. We will have a loop ASAP.

[rishabhpoddar]

This has been fixed in >= v0.8.2.

Thanks @MicahParks for highlighting this issue and suggesting solutions for it. We went for the second approach, but have not yet implemented a way to allow users to end background goroutines cause we don't really see a use case for people wanting to end it for sign in methods - do you have a use case in mind?

[MicahParks]

I haven't tested it myself, but the change looks like it does resolve the issue using the second suggestion. Perhaps a sync.RWMutex would improve performance for the PR: #156

a way to allow users to end background goroutines cause we don't really see a use case for people wanting to end it for sign in methods - do you have a use case in mind?

If you'd like to, or have already, exposed a way for the package user to release all resources for your package or specifically the Apple/Google Workspaces functionality, then that would be the use case I have in mind.

However, I know many people work under the theory that some services "run forever", or at least for the lifetime of the program. I also subscribe to that theory at times, so I think it's really up to you to determine if that's a part of your use case. Make sure to keep the background goroutine in mind if a future use case involves cleaning up resources. 👍