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

[lidel]

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:

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
   • Sample 1: upload single file: upload-multiple-files-via-browser-ipfs-api-bug-demo.zip
     • This should produce error
   • Sample 2: upload these three files at once
     • This should produce error

[Stebalien]

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]

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

[Stebalien]

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]

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]

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]

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

[lidel]

@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]

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]

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

Linking to workarounds:

• ipfs-companion: created PR #509 which fixes the header in requests made to /api/v0/add.
  • It does not modify requests sent to all APIs, only one used by ipfs-companion
  • It works only in Firefox: Chrome does not allow modification of Connection header (Unable to upload to IPFS API: passing multiple files to ipfs.files.add sometimes fails ipfs-companion#480 (comment))
• js-ipfs-api: may not be possible to fix in browsers, details in https://github.com/ipfs/js-ipfs-api/issues/797#issuecomment-401469158

[lidel]

@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]

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.

[hsanjuan]

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]

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]

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.

[Stebalien]

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]

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

[hsanjuan]

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

[lidel]

@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.

[Stebalien]

Fixed in master.

[Stebalien]

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