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

Discussion: Reconsidering requestor pausing #347

Open
hannahhoward opened this issue Feb 4, 2022 · 2 comments
Open

Discussion: Reconsidering requestor pausing #347

hannahhoward opened this issue Feb 4, 2022 · 2 comments
Assignees
Labels
need/triage Needs initial labeling and prioritization

Comments

@hannahhoward
Copy link
Collaborator

hannahhoward commented Feb 4, 2022

Currently, we have an implementation of requestor pausing that works as follows:

  • Requestor can be paused in a block hook or a imperatively (calling PauseRequest on graphsync with a request ID)
  • "Pausing" works by simply cancelling the request. "Resuming" works by resending the request with DoNotSendFirstBlocks

IMHO, this is ridiculous. The reason I did it is that I wanted the requestor to stop accepting blocks entirely as soon you paused. This once upon a time approach was based on trying to get payment channels to work for data transfer push requests. (see #346 for explanation).

I believe the proper way to do this is:

Questions:

  • does this make sense?
  • should this require some kind of validation? when I download stuff over HTTP, I don't seem to require anyone's auth to pause & resume?
  • what happens if the responder never sends back a pause?
  • should a responder time out a paused request that was paused by the requestor after certain amount of inactivity? (protect against dosing with lots of paused requests)
  • should we remove pausing on the requestor side entirely -- move it to simply sending update requests with extensions and letting the responder pause if it wants to?
  • What about requestor resuming? Do we need a resume request type as well?

Why should we have imperative requestor pausing?

  • I'm not sure it's used today, but it certainly makes sense from a the standpoint of being like HTTP. If one imagines a download manager, one certainly imagines the ability to hit a pause button
@hannahhoward hannahhoward added the need/triage Needs initial labeling and prioritization label Feb 4, 2022
@rvagg
Copy link
Member

rvagg commented Feb 4, 2022

  • makes sense
  • I don't see a case off hand for validation, maybe there is one but I don't have the markets coverage to see it?
  • a PauseRequest() call could also have a timeout on the caller side, such that if the timeout is reached and the responder doesn't send back a pause, a forced cancel is initiated and an error propagated—seems appropriate for a misbehaving responder, no?
  • definitely yes there should be a timeout, you're asking for not just a connection to remain active but a lot of state to remain in memory—this is a big problem with http/2, maybe it'd be worth looking in to how that's being dealt with (I tuned out on that stuff a couple of years ago but it was interesting to watch the challenges of state management and easy DoS vectors)
  • for resume, we either need to subtly rename "New" or introduce a new type that's obvious (this is where "Play/Pause" works nicely in the AV world). Maybe "New" should actually be "Start"?

@hannahhoward
Copy link
Collaborator Author

Upon further reading, I have learned that HTTP's pause/resume works essentially exactly like GraphSyncs -- the client cancels the request, then sends a second range request to resume. As many often say "HTTP is a stateless protocol".

I guess it can be for graphsync as well :P

I think a better question is how to do better versions of our start / stop extensions -- we started with DoNotSendCids, then moved to DoNotSendFirstBlocks, and I think the next thing would be to support "start traversal at path" -- ideally in a way that server could pick up without maintaining state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/triage Needs initial labeling and prioritization
Projects
None yet
Development

No branches or pull requests

3 participants