Skip to content
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

jwkCache can't be updated concurrently #65

Closed
dusty-phillips opened this issue May 11, 2024 · 3 comments · Fixed by #88
Closed

jwkCache can't be updated concurrently #65

dusty-phillips opened this issue May 11, 2024 · 3 comments · Fixed by #88
Assignees
Labels
bug Something isn't working

Comments

@dusty-phillips
Copy link

dusty-phillips commented May 11, 2024

Last night my service failed with this traceback:

fatal error: concurrent map writes
goroutine 20214 [running]:
github.com/passageidentity/passage-go.(*App).fetchJWKS(0xc00017a390)
	/opt/render/project/go/pkg/mod/github.com/passageidentity/[email protected]/authentication.go:70 +0xb8
github.com/passageidentity/passage-go.New({0xc00003a33f, 0x18}, 0x0?)
	/opt/render/project/go/pkg/mod/github.com/passageidentity/[email protected]/app.go:41 +0x185
       <snip>

This is because fetchJWKS() is writing to the jwkCache without synchronization: https://github.com/passageidentity/passage-go/blob/main/authentication.go#L70C2-L70C10

I think one of these changes is needed:

  • This code needs to be wrapped in a lock or mutex (the reads may need them as well)
  • The jwkCache global (defined here) needs to be converted to e.g. sync.Map or similar.
  • The cache needs to be stored per-app instead of global
  • The documentation needs to be updated to encourage users to create a single app instance (using e.g. OnceValue) instead of one per request.

Great product and service, BTW. Your dev experience blows the competition out of the water.

@dusty-phillips
Copy link
Author

This happened again after I tried a singleton app instance, so I think the fourth option doesn’t work.

@bertrmz
Copy link
Contributor

bertrmz commented May 15, 2024

Hi @dusty-phillips, thank you for raising this issue.

We're working on a fix; I'll post an update here when it's available.

@ctran88 ctran88 self-assigned this Oct 17, 2024
@ctran88
Copy link
Contributor

ctran88 commented Oct 17, 2024

Hi @dusty-phillips very sorry it's been so long!

#88 should address his issue. Once it's merged in and published I will update here with the fixed version.

@ctran88 ctran88 linked a pull request Oct 22, 2024 that will close this issue
12 tasks
@ctran88 ctran88 added the bug Something isn't working label Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants