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

Accept/Reject requests up front #384

Merged
merged 6 commits into from
Jun 25, 2022

Conversation

hannahhoward
Copy link
Collaborator

Goals

This PR addresses a long standing design issue with processing incoming requests to support filecoin-project/go-data-transfer#310. Essentially, when requests come in, we queue them for processing, even if the request will ultimately be rejected.

This seems like poor behavior: under heavy load, a client might wait minutes simply to be told they won't get the response they wanted. This leads to very usual behavior as well up the stack -- data transfer requests can sit in a "requested" state for a very long time for example.

It also means we're wasting resources on holding data on requests we ultimately aren't going to process.

Implementation

  • remove request queued hooks
  • move the AugmentContext function on request queued hooks to the request hooks
  • add a new listener (no hook) to handle the moment when requests begin processing
  • call request hooks immediately at the moment an incoming request is made
  • if the request is:
    • accepted: queue as normal
    • rejected for any reason: don't queue, only hold request data till the request failure finishes sending
    • paused: don't queue (previously, we'd queue, dequeue, run request hooks, and then do nothing but put the request in a paused state -- eek)
  • remove the query preparer struct which is now just a pure function
  • construct the traverser directly when we first start processing the request
  • add a couple additional pieces of data in the response struct so we can construct the traverser later

For discussion

While I am pretty sure this is the right change -- in fact I'm very sure -- it's none the less significant as it makes all requests get rejected or accepted immediately. Generally, I think this has better DDOS implications -- we don't hold around requests we won't process for a long time. Nevertheless, perhaps it's more risky in certain situations -- if you get DDOS'd with requests, and you have very slow hooks, possibly it takes longer to process? Ultimately, we're processing requests immediately anyway, and now we're just running a slightly different set of hooks, so I'm not super worried here. Nevertheless, it merits discussion.

it makes much more sense to reject requests early than put them in the processing queue. While there
may be a minimal cost to evaluating a hook, there is no reason to wait to send a rejection
add test to verify response processing listerner called
responsemanager/server.go Outdated Show resolved Hide resolved
responsemanager/server.go Outdated Show resolved Hide resolved
@rvagg
Copy link
Member

rvagg commented Jun 9, 2022

Should RegisterIncomingRequestProcessingListener and RegisterOutgoingRequestProcessingListener get some test coverage?

add test for hooks process (issue caught and fixed in process) and address PR comments
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the additional comments in there, they're helpful

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