search

ipld / edelweiss

Decentralized Protocol Compiler
Other
16 stars 6 forks source link

fix reading multiple responses #44

Closed petar closed 2 years ago

petar commented 2 years ago

Addresses https://github.com/aschmahmann/someguy/issues/1

aschmahmann commented 2 years ago

@petar on the server end there also seems to be a need for flushing the HTTP writes or they don't actually make it to the other side. Discovered by testing this in someguy.

Perhaps adding some code like this after each write would do the job. We should probably add a test for this as well.

if f, ok := writer.(http.Flusher); ok {
    f.Flush()
}
petar commented 2 years ago

@petar on the server end there also seems to be a need for flushing the HTTP writes or they don't actually make it to the other side. Discovered by testing this in someguy.

Perhaps adding some code like this after each write would do the job. We should probably add a test for this as well.

if f, ok := writer.(http.Flusher); ok {
  f.Flush()
}

I've added the flush. It's not very clear how to test this though. I already had a test for multiple responses using the httptest.NewServer, which passes without the flush. Was there anything special about the setup where you ran into this? If it's just a vanilla HTTP/TCP connection, we could try adding a test that uses http.NewServer (as opposed to httptest.NewServer).

aschmahmann commented 2 years ago

It's not very clear how to test this though.

Have a data source for records where it'll give you the first record and for the second record it just stalls (and therefore the server never returns). See if the client actually gets a record or not. If it does then return (and cancel contexts), if it doesn't return after a while (e.g. a second) then error.

Was there anything special about the setup where you ran into this?

The data source my server was streaming from had asynchronous outputs that took a while to complete (i.e. DHT queries that give provider record results almost immediately but spend a while completing the query to look for more provider records).

petar commented 2 years ago

@aschmahmann I've added the test. While working on it, I noticed another behavior to be conscious of: If client A connects to server B and has a long-running connection (e.g. multiple slow async responses), then another concurrent request from client A to B may stall, I think because Go's HTTP library tries to reuse the connection, but it is busy. So when Reframe is used, the HTTP stack should be explicitly configured to not reuse connections!