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

Add support for context counter #13

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

creack
Copy link
Contributor

@creack creack commented Mar 19, 2022

As I needed a networked-based limiter with transactions, I had to add the context to httprate.

Initially made the change locally, copy/pasting most of the repo, but might as well push upstream.

It is fully backward compatible.

@VojtechVitek
Copy link
Contributor

VojtechVitek commented Jul 24, 2024

Hi @creack,

I'm just looking into this PR and it looks good. Thank you!

However, I think we should combine the changes with ideas from #2, if we're to expand the LimitCounter interface.

As I needed a networked-based limiter with transactions, I had to add the context to httprate.

Can you please describe your use-case a bit more? Do you use redis as the LimitCounter or something else?

(And would you be able to GetAndIncr in a single atomic operation?)

@VojtechVitek
Copy link
Contributor

VojtechVitek commented Jul 25, 2024

@creack I'm thinking about the pros/cons of passing the context timeout down to the networked-based limit counter(s).

Few things to consider:

  • Would it be enough if we set a global timeout in the .Get() and .IncrementBy() implementation without considering request.Context() timeout?
    • e.g. if redis doesn't respond within 50ms, we fall back to a local in-memory counter, no matter if client disconnected?
  • If there was a DDOS attack, e.g. thousands of HTTP clients connecting and disconnecting within <1ms .. should we count them in towards the HTTP 429 limit? Or should we "cancel" the request to network-based limiter and let the attacker continue?

@bendrucker
Copy link

I came across this with a related need for context propagation that I can describe in further detail if needed: tracing.

If you're using httprate-redis and wrap your Redis client with tracing, the Redis spans aren't attached to the parent. Tracers in Go rely on threading the active span through with context.WithValue. I happen to be using Datadog's dd-trace-go library and hook for go-redis but this is functionally equivalent:

https://github.com/redis/go-redis/tree/master/extra/redisotel

A context version of IncrementBy should be included too—httprate-redis uses that.

Some thoughts on your DDOS question @VojtechVitek:

Or should we "cancel" the request to network-based limiter and let the attacker continue?

As a last resort, it's up to the limiter implementation to respect the context, and it can opt to ignore cancelation. In theory you could a context to pass tracing context but ignore the cancelation signal.

Or, there's the option to add an option to httprate to wrap contexts in WithoutCancel before passing them:

https://pkg.go.dev/context#WithoutCancel

If that were the default behavior it would be truly non-breaking. Tracing would work by default but cancelation could require opt-in. context.WithoutCancel requires Go 1.21 but the type withoutCancelCtx it returns can be easily inlined for earlier versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants