ipfs / go-graphsync

Initial Implementation Of GraphSync Wire Protocol
Other
100 stars 38 forks source link

Accept/Reject requests up front #384

Closed hannahhoward closed 2 years ago

hannahhoward commented 2 years ago

Goals

This PR addresses a long standing design issue with processing incoming requests to support https://github.com/filecoin-project/go-data-transfer/issues/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

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.

rvagg commented 2 years ago

Should RegisterIncomingRequestProcessingListener and RegisterOutgoingRequestProcessingListener get some test coverage?