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

P2P HTTP Implementation: open problems #6042

Closed
algorandskiy opened this issue Jun 28, 2024 · 4 comments
Closed

P2P HTTP Implementation: open problems #6042

algorandskiy opened this issue Jun 28, 2024 · 4 comments
Labels
p2p Work related to the p2p project

Comments

@algorandskiy
Copy link
Contributor

algorandskiy commented Jun 28, 2024

There are a few open questions about p2p http risen in the p2p feature branch merge PR:

  1. If the ledgerService / catchpoint downloader grabs GetHTTPRequestConnection and grabs the underlying stream, does this mean it will break any other HTTP requests that are already using that stream (or might use it concurrently)? Just trying to understand what happens when you give away the underlying streams like this

Originally posted by @cce in #5939 (comment)

  1. Maybe in a follow up PR we should get rid of StreamChainingHost and split by using a dedicated protocol for downloading ledger files or something? Just wondering whether it is possible with getStream() to end up with two HTTP requests trying to write/read from the same underlying stream at the same time?

Originally posted by @cce in #5939 (comment)

  1. An idea for the future, maybe RegisterHTTPHandler sets up a separate protocol for each handler, rather than all sharing algorandP2pHTTPProtocol with our own gorrila mux router on top? Then I think it would be better able to support support concurrent HTTP requests for different handlers over different streams? Or maybe the libp2p http stuff already provides for multiple concurrent HTTP reqs? I was imagining they all share a single stream, but maybe that's not true.

Originally posted by @cce in #5939 (comment)

@algorandskiy algorandskiy added the p2p Work related to the p2p project label Jun 28, 2024
@algorandskiy
Copy link
Contributor Author

algorandskiy commented Jun 28, 2024

AFAICT most of the code added to the RequestTracker from #1390 (and mirrored in feature/p2p for p2p http conns) can be replaced with two lines by the Go 1.20 ResponseController feature https://tip.golang.org/doc/go1.20#http_responsecontroller like in their example — vastly easier than managing a conn tracking middleware system

The ResponseController type provides a clearer, more discoverable way to add per-handler controls. Two such controls also added in Go 1.20 are SetReadDeadline and SetWriteDeadline, which allow setting per-request read and write deadlines. For example:

func RequestHandler(w ResponseWriter, r *Request) {
  rc := http.NewResponseController(w)
  rc.SetWriteDeadline(time.Time{}) // disable Server.WriteTimeout when sending a large response
  io.Copy(w, bigData)
}

@algorandskiy
Copy link
Contributor Author

algorandskiy commented Sep 18, 2024

Items (1) and (2) were fixed in #6044

@algorandskiy
Copy link
Contributor Author

I guess item (3) can be taken as an enhancement for p2p milestone 2.

@gmalouf
Copy link
Contributor

gmalouf commented Sep 19, 2024

Captured for milestone 2

@gmalouf gmalouf closed this as completed Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2p Work related to the p2p project
Projects
None yet
Development

No branches or pull requests

2 participants