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

If a command output writer writes 4096 bytes before it completed reading it's input, it won't be able to read the rest when using the HTTP API #3367

Closed
keks opened this issue Nov 7, 2016 · 9 comments
Labels
kind/bug A bug in existing code (including security flaws) topic/api Topic api

Comments

@keks
Copy link
Contributor

keks commented Nov 7, 2016

I stumbled upon this when researching #3336.

When the HTTP API receives a request, it starts the command using the HTTP arguments as arguments to the commands and feeds the request body to the command "as stdin". The command also has an stdout, which is an io. Reader on the caller's side.
When you write to an http.ResponseWriter, all unconsumed data in the request body is lost.
To send the command result to the client, flushCopy is called with r being the command output and w being the http.ResponseWriter.

At the end of the first iteration, the connection is being flushed and the response headers are sent. If the request has not been fully consumed until then, it will be truncated.
The problem is that the information whether the input has been consumed is not available inside flushCopy, so to fix this we need to find a way do pass it.

The reason I think this is important is that at the moment when you subscribe a pubsub topic they headers are not being sent until that Flush() after the read is called. This way the all Go clients will block when sending their request, because they wait for the headers.
That is also the reason I added a Flush at the loop top in the first place. I wasn't aware that you couldn't read the body afterwards.

@keks
Copy link
Contributor Author

keks commented Nov 7, 2016

What I already tried is that I wrapped the http Request.Body and ResponseWriter such that all Writes, Flushes and WriteHeaders will block until r.Body.Read() returns EOF. However, this only made the code block eternally. I'll see what other options I have. I guess contexts are another option, but I'm not sure.

@Kubuxu
Copy link
Member

Kubuxu commented Nov 8, 2016

That is a major flow as it prevents us from streaming the input and the output.

Is there any solutions for that or is it limitation of http protocol?

cc @lgierth @whyrusleeping

@keks
Copy link
Contributor Author

keks commented Nov 8, 2016

AFAIK this is a limitation of the HTTP protocol. From the HTTP point of view this makes sense as you won't be able to tell whether a request is valid or not if you didn't read it completely.

@whyrusleeping
Copy link
Member

So does this mean that a sufficiently large ipfs add would break?

@whyrusleeping
Copy link
Member

Actually, how does ipfs add even work at all then?

@keks
Copy link
Contributor Author

keks commented Nov 10, 2016

Hmm, it seems to work with ipfs add. I wonder what the problem was, then...I'm investigating.

@keks
Copy link
Contributor Author

keks commented Nov 10, 2016

Okay, I think I got it.

It is okay to read after writing or flushing if you use multipart requests and call "http".Request.MultipartReader. However "multipart".Reader.NextPart needs to be called at least once before the first flush, otherwise we end up with an "http".ErrBodyReadAfterClose, which is what happened in #3336.

@whyrusleeping whyrusleeping added topic/api Topic api kind/bug A bug in existing code (including security flaws) labels Nov 11, 2016
@whyrusleeping
Copy link
Member

@keks Ah, okay. So we should do some special handling there, and make it clear that you should use multipart if you intend on being in that scenario.

@keks
Copy link
Contributor Author

keks commented Nov 14, 2016

re #3304 (comment):

Maybe we should write a small guide on 'how to write a new ipfs command' ?

Hmm, I never wrote an ipfs command and I'm not sure how often this will be done. I propose adding a source code comment attached to the Run function in the Command struct definition such that it will be shown in godoc. I'll post a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws) topic/api Topic api
Projects
None yet
Development

No branches or pull requests

4 participants
@keks @whyrusleeping @Kubuxu and others