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 ability to abort the response #155

Merged
merged 5 commits into from
Mar 9, 2025

Conversation

pavelsavara
Copy link
Contributor

  • refactor ResponseOptions parameters
  • add ability to abort the response

- add ability to abort the response
@aaronpowell
Copy link
Owner

Is the intent of this to simulate longer running requests or requests that are only partially successful (ie - aborted)? Also, isn't having an aborted request going to be a false representation of the status code that is being returned, like, using the abort during body pipeline would result in (potentially) invalid JSON being returned on a 200 OK response, which kind of suggests that it wasn't really OK after all.

My other concern is around adding more sleeps as that can reduce the concurrent requests that can be processed. I've seen malicious traffic spikes in the past in which the max sleep is set which does mean that the server has a lot of awaited threads and becomes really slow.

@pavelsavara
Copy link
Contributor Author

pavelsavara commented Feb 28, 2025

I'm just scratching my itch locally with this change and wanted contribute back. I don't mind if this is not merged :)
But I'm willing to make small improvements if this is useful PR.

Is the intent of this to simulate longer running requests or requests that are only partially successful (ie - aborted)?

I'm testing edge-cases of browser behavior in case of server abort/disconnect.

Also, isn't having an aborted request going to be a false representation of the status code

Yes, the simulated scenario is server wanted to return OK 200, but something went wrong (like mobile network disconnect or Ethernet cable unplugged). In such cases the raw partial payload on the wire starts with 200 header, but doesn't finish.
It's up to HTTP client to fail in proper way (which is what I'm testing).

I've seen malicious traffic spikes in the past in which the max sleep

Hmm, there is already one such sleep and that's enough for such DDOS. My PR doesn't really change the situation.

I put few sleeps with constant 10ms, to make sure the bytes are on the wire before I abort.
Is there something better I can do ? Like check that bytes departed before I sleep ?

Possible remedy could be to clamp the sleep to some maximum. Ideally low value, like 500ms, which is good enough for automated testing. But for local stepping in the client code in debugger, sometimes you need minutes. So maybe clamping it just for the online site and keep it unlimited for the localhost.

Is there #if or configuration or environment variable we can use to detect we are the live site ?

Done ^^

Further improvement could be some DDOS protection based on source IP address. I not willing to work on that.

@pavelsavara
Copy link
Contributor Author

Now I see there is FairUseRateLimiterExtensions and also that DoSleep clamps to 5 minutes already.

@pavelsavara
Copy link
Contributor Author

Is there #if or configuration or environment variable we can use to detect we are the live site ?

I think app.Environment.IsDevelopment() would suffice. I made the change.

I also implemented body dribbling - responding with first 20 characters on separate packets.

@aaronpowell
Copy link
Owner

Now I see there is FairUseRateLimiterExtensions and also that DoSleep clamps to 5 minutes already.

Unfortunately the rate limiter isn't enabled as I turned it on and it broke everything and I can't find enough docs on how to make it actually work 🤣

@aaronpowell
Copy link
Owner

Is there #if or configuration or environment variable we can use to detect we are the live site ?

I think app.Environment.IsDevelopment() would suffice. I made the change.

On a second read through, I think you're right that it's not really adding any additional sleep overheads. So I'd prefer to leave the max sleep where it was for prod as that has been the expected for a few years now so I wouldn't want a regression/major change like that.

@pavelsavara
Copy link
Contributor Author

So I'd prefer to leave the max sleep where it was

Reverted

@aaronpowell aaronpowell merged commit 1672ae8 into aaronpowell:main Mar 9, 2025
1 check failed
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.

2 participants