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

Error while files.add over HTTP API (http: invalid Read on closed Body) #5168

Closed
lidel opened this issue Jun 28, 2018 · 20 comments · Fixed by ipfs/go-ipfs-cmds#116
Closed

Error while files.add over HTTP API (http: invalid Read on closed Body) #5168

lidel opened this issue Jun 28, 2018 · 20 comments · Fixed by ipfs/go-ipfs-cmds#116
Assignees
Labels
kind/bug A bug in existing code (including security flaws) topic/commands Topic commands

Comments

@lidel
Copy link
Member

lidel commented Jun 28, 2018

This bug was originally found in ipfs/ipfs-companion#480.
Since then I was able to confirm issue originates between js-ipfs-api and HTTP API exposed by go-ipfs and created a demo app that reproduced issue on two separate machines.

Version information:

OS: two Linux boxes (one was Gentoo second was Debian)

IPFS Node:

go-ipfs version: 0.4.16-rc1-b183da3
Repo version: 7
System version: amd64/linux
Golang version: go1.10.3

HTTP API Client: js-ipfs-api 22.1.1 running in browser context

Type:

Bug

Note: This issue is about error in response produced by go-ipfs. There is a separate bug for the lack of proper handling of partial errors in js-ipfs-api at https://github.com/ipfs/js-ipfs-api/issues/797)

Description:

Sometimes (for specific payloads in browser context) the HTTP API for ipfs.files.add provided by go-ipfs returns the http: invalid Read on closed Body error.

It is especially problematic when adding multiple files at once, because error is returned in the middle of response, along with successful entries with the same payload:

screenshot_18

How to Reproduce

The issue does not seem to appear when curl is used, it seems to be specific to browser context and the way js-ipfs-api makes requests to HTTP API.

I've found two ways to reproduce/investigate it:

A) By inspecting recorded HTTP interaction

Below is a recorded HTTP Archive (HAR) of interaction that produced the error:
Archive%2018-06-28%2015-34-47.har

Interaction was uploading these sample files with demo app (details below).

B) By Using Demo App

Turns out the .zip with demo app triggers the issue 🙃 , so its easy to reproduce:

  1. Expose API of go-ipfs 0.4.15 or 0.4.16-rc1 locally on port :5001
  2. Download demo app:upload-multiple-files-via-browser-ipfs-api-bug-demo.zip
    (uses ipfs-api ^22.1.1 and uploads files via ipfs.files.add with wrapWithDirectory: true)
  3. Build it and start via npm install && npm start, then open form at http://localhost:3000
  4. Reproduce using samples known to trigger issue at go-ipfs
@Stebalien
Copy link
Member

Looks like a go bug: golang/go#15527

As far as I can tell, work around is to avoid flushing the response till we finish reading the request.

@Stebalien Stebalien added the kind/bug A bug in existing code (including security flaws) label Jun 28, 2018
@Stebalien
Copy link
Member

So, I'm pretty sure that work around is incorrect (although it appears to work). However, the diagnosis is definitely correct.

@Stebalien
Copy link
Member

This may also explain why our progress meters are broken. It may also explain a bunch of random errors and why some people have trouble adding large datasets to go-ipfs.

@Stebalien
Copy link
Member

Stebalien commented Jun 29, 2018

Ah. So, this has worked because we almost always use chunked encoding (which doesn't trigger the issue).

*edit: this worked because the client was sending "connection: close".

@Stebalien
Copy link
Member

So, I'm not sure how to work around this. We rely on being able to stream status information while processing large requests. We usually don't notice this issue because the go-ipfs CLI usually makes a single HTTP requests and sets Connection: close however, this isn't something we're going to be able to control from the browser. Really, we don't really want to be closing connections from the browser (or other complex applications) for performance reasons. The only solutions I can see are:

  1. Fork net/http and somehow try to maintain compatibility with the rest of the net/http ecosystem. I tried this. It started getting real ugly.
  2. Use a different http library. I can't find any good ones that are even reasonably compatable.
  3. Disable keep alive on the server (bad performance).

I get the impression it'll be a while before go fixes this bug.

Stebalien added a commit that referenced this issue Jun 29, 2018
This disables keeping http connections alive in our API and Gateway servers. It
"fixes" #5168 but we really don't want to do this in practice.

License: MIT
Signed-off-by: Steven Allen <[email protected]>
@Stebalien
Copy link
Member

@lidel try the fix/disable-keepalives branch if you need something to work with while we try to find a better fix.

@Stebalien Stebalien added the topic/commands Topic commands label Jun 29, 2018
@lidel
Copy link
Member Author

lidel commented Jun 29, 2018

@Stebalien thank you for looking into this. With this info I strongly suspect the bug is responsible for a bunch of random errors users of HTTP API in browser context been reporting over past two years.

I was able to confirm workaround from fix/disable-keepalives branch "solves" the issue, unfortunately most of users won't be able to run custom build.

Edited: I was too optimistic: disabling `&progress` does not work as a reliable workaround, it just changes the conditions under which issue occurs (some files started working, others stopped).

However, you mentioned streaming status information, so I looked at bidirectional aspect of `files.add` and was able to confirm a viable workaround for the client side is to simply _disable progress reporting_.

Interaction samples with/without progress reporting:

  • ERROR with &progress=true: HAR (http: invalid Read on closed Body)
  • OK without &progress: HAR

Not ideal (we lose progress bars etc) but at least upload will work reliably.

@Stebalien
Copy link
Member

Beyond "not ideal", it's also not a very reliable solution. What happens if you add a Connection: close header to all requests? That may be a viable "solution" until we can fix this.

@lidel
Copy link
Member Author

lidel commented Jun 29, 2018

After playing a bit with setting Connection: close on the request it I can confirm it.. seems to work reliably.

Linking to workarounds:

lidel added a commit to ipfs/ipfs-companion that referenced this issue Jul 2, 2018
This PR:

- provides a fix for a bug described in https://github.com/ipfs-shipyard/ipfs-companion via workaround from ipfs/kubo#5168 (comment)
   - **TL;DR** Browser extension is able to detect requests to `/api/v0/add` and set `Connection: close`
- moves core upload logic from background page to `quick-upload` page
@lidel
Copy link
Member Author

lidel commented Jul 5, 2018

@Stebalien ok, this is quite serious bug: setting Connection: close is not something javascript clients can do. WebExtension APIs are a mixed bag: Chrome also does not allow any change to this header, so random uploads remain for Chrome users of our browser extension as well.

Are we able to introduce some sort of opt-in, header-based workaround (that could be added to js-ipfs-api to fix it for everyone), or is it too deep in the http stack?

Something like:

  1. Clients sends request to /api/v0/api with additional,
    custom HTTP header X-Ipfs-Connection: close
  2. go-ipfs acts like it received Connection: close

@Stebalien
Copy link
Member

Really, we probably want to just configure the API's HTTP server to always close connections. The branch I pointed you towards does this for the gateway as well but we definitely don't want that.

WouterGlorieux added a commit to ValyrianTech/ValyrianSpellbook that referenced this issue Jul 18, 2018
these methods are added as a temporary workaround because adding an entire dir
to IPFS can cause a bug: ipfs/kubo#5168

add_zipped_dir will zip a directory and upload the zipfile to IPFS
get_zipped_dir will download a zipped dir and extract it into a target directory
hsanjuan added a commit to ipfs-cluster/ipfs-cluster that referenced this issue Aug 18, 2018
See discussion in ipfs/kubo#5168

We cannot stream responses with keep-alives enabled.

I prefer this to not be a client feature, as otherwise users might end up
shooting themselves in the foot.

Note, the price is a corrupted request body which gets added
normally and gives wrong hashes!

License: MIT
Signed-off-by: Hector Sanjuan <[email protected]>
@hsanjuan
Copy link
Contributor

I have spent hours today on exactly this same issue. It appears consistently but randomly (i.e. a test fails all the time, but a very similar test of in a different place works fine, and then things get fixed by just changing some code (playing with Flush())). The result for me is actually getting a corrupted request body, resulting in the multipart reader reading different data from it and adding different content (but not failing, which is very scary).

And I got to the same conclusion, the workarounds are:

  • Server.SetEnableKeepAlives(false) or
  • request.Close = true in the client

Alternatively, I guess the /add endpoint might simply not stream anything and read the full request first. The http docs do mention that Go's HTTP/1.x implementation does not support full-duplex request bodies being written while the response body is streamed, and we're essentially doing it.

Note that for browsers, I have read that using no-cache, no-sniff and text/event-stream is probably a good thing.

Also, this one seems related too: golang/go#20528

@ianopolous
Copy link
Member

I've hit the exact same problem from the Java http api, and got a small test to reproduce it randomly (~1 in 5 fails): https://github.com/ipfs/java-ipfs-api/blob/master/src/test/java/io/ipfs/api/AddTest.java
Similarly, repeating the same request from curl works 100% of the time.

I've got it to work 100% in Java too by adding the following header:
Expect: 100-continue

@lidel
Copy link
Member Author

lidel commented Aug 31, 2018

That is a good news for IPFS Companion @ianopolous !
It was not possible to modify Connection header in webextension running in Chrome, so I am switching to Expect workaround which fixes the problem for uploads in both Firefox and Chrome.

lidel added a commit to ipfs/ipfs-companion that referenced this issue Sep 3, 2018
Workaround for 'invalid Read on closed Body' in files.add (webext in Chrome)

> Chrome does allow setting `Expect` via [`onBeforeSendHeaders`](https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/onBeforeSendHeaders) WebExtension API

This means we now have a workaround for #480 that works in both Firefox and Chromium.

Details: #480 (comment) & ipfs/kubo#5168 (comment)

Closes #480
@Stebalien
Copy link
Member

Fix here: ipfs/go-ipfs-cmds#116

It manages to only break the connection if there's a body to read (at least it should). I've tested it with @lidel's app and it appears to work.

Could test out that PR?

@lidel
Copy link
Member Author

lidel commented Sep 16, 2018

@Stebalien my gx is bit rusty, what are the steps to rebuild go-ipfs with that dependency?

@hsanjuan
Copy link
Contributor

@lidel gx-go uw; gx update go-ipfs-cmds <hash>; gx-go rw; make install should do it.

@lidel
Copy link
Member Author

lidel commented Sep 21, 2018

@Stebalien tested fix from ipfs/go-ipfs-cmds#116 (disabled Companion's workarounds for the duration of the test) and it solves the issue for all samples that were known to me to trigger the error.

hsanjuan added a commit to ipfs-cluster/ipfs-cluster that referenced this issue Sep 29, 2018
This is a workaround to have clients behave properly with the /add
endpoint by asking them to close connections when done, effectively
disabling keep-alive for this.

This means we don't need to disable keep-alives fully on all servers,
since the rest of endpoints are not affected (they are not streaming
endpoints).

Reference ipfs/kubo#5168

License: MIT
Signed-off-by: Hector Sanjuan <[email protected]>
@Stebalien Stebalien reopened this Dec 13, 2018
@Stebalien
Copy link
Member

Fixed in master.

@Stebalien
Copy link
Member

This has been fixed! golang/go#15527 (comment)

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/commands Topic commands
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants