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

http: Replace ConcurrencyLimiter with IDLocker #2925

Merged
merged 10 commits into from
Apr 5, 2023

Conversation

mastercactapus
Copy link
Member

@mastercactapus mastercactapus commented Apr 4, 2023

Description:

  • Replaces the usage on ConcurrencyLimiter with IDLocker.
  • Updated HTTPError to handle rate-limits
  • Added benchmarks for IDLocker
  • Adds limit on concurrent Basic auth requests (per username)

Which issue(s) this PR fixes:
A follow-up to #2924

Out of Scope:
Deleting the ConcurrencyLimiter will be done in a separate PR to keep size/context down.

Additional Info:

goos: linux
goarch: amd64
pkg: github.com/target/goalert/app
cpu: AMD Ryzen 9 5900HX with Radeon Graphics  
BenchmarkConcurrencyLimiter_Sequential-16                	 4598095	       287.7 ns/op	      64 B/op	       2 allocs/op (1338.26MB)
BenchmarkConcurrencyLimiter_Sequential_Cardinality-16    	 1583104	       845.8 ns/op	     386 B/op	       8 allocs/op (955.74MB)
BenchmarkConcurrencyLimiter_Concurrent-16                	  708006	      1672 ns/op	     179 B/op	       3 allocs/op (315.64MB)
BenchmarkConcurrencyLimiter_Concurrent_Cardinality-16    	  341535	      3381 ns/op	     283 B/op	       6 allocs/op (140.26MB)

goos: linux
goarch: amd64
pkg: github.com/target/goalert/ctxlock
cpu: AMD Ryzen 9 5900HX with Radeon Graphics  
BenchmarkIDLocker_Sequential-16                	23114173	        50.12 ns/op	       0 B/op	       0 allocs/op (78.61MB)
BenchmarkIDLocker_Sequential_Cardinality-16    	11040678	       113.2 ns/op	       7 B/op	       0 allocs/op (84.63MB)
BenchmarkIDLocker_Concurrent-16                	  815606	      1457 ns/op	     130 B/op	       1 allocs/op (210.60MB)
BenchmarkIDLocker_Concurrent_Cardinality-16    	  594417	      1978 ns/op	      68 B/op	       1 allocs/op (54.25MB)

@mastercactapus mastercactapus marked this pull request as ready for review April 4, 2023 21:36
We don't want to enable queuing, just limit concurrent attempts
@github-actions github-actions bot added the size/l label Apr 4, 2023
@m17ch m17ch self-requested a review April 5, 2023 16:54
@m17ch m17ch merged commit 0f78816 into master Apr 5, 2023
@m17ch m17ch deleted the replace-concurrencylimiter branch April 5, 2023 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants