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

Unable to upload to IPFS API: passing multiple files to ipfs.files.add sometimes fails #480

Closed
lidel opened this issue May 18, 2018 · 3 comments
Labels
help wanted Seeking public contribution on this issue kind/bug A bug in existing code (including security flaws) status/blocked/upstream-bug Blocked by upstream bugs status/ready Ready to be worked

Comments

@lidel
Copy link
Member

lidel commented May 18, 2018

Background

I was playing with ipfs.files.api over js-ipfs-api and go-ipfs (PR #479) and noticed an odd edge case. Sometimes, for specific payloads, go-ipfs returns an error in the middle of payload:

{"Name":"T-RexFaceEmoji.jpg_original","Hash":"QmR1G8Jq4rNiYwCKyNWEZhNS7i8EWrCpz2xDjKsB9ofi82","Size":"23723"}
{"Name":"FlyingSaucerEmoji.jpg_original","Hash":"QmVWxHcLVHYpFJP6R1ZLBZcpAZsxyFMfU7BU84D2LAMSed","Size":"13071"}
{"Message":"http: invalid Read on closed Body","Code":0,"Type":"error"}

Then, upon receiving this response, js-ipfs-api does not throw an error, but silently ignores the last line and returns a partial result list with only two first items.

How to reproduce

I was able to reproduce it with brand new (dockerized) node and latest Companion from #479, so I assume the bug is real, but would be great if someone else was able to reproduce and comment if they got the same result:

  1. Download three files from:
    https://ipfs.io/ipfs/QmUGmyGCJEntjyxgcBmptkZ34k7PsgnCzXPoqwiWBDTK6L and save them on your disk
  2. Start local instance of go-ipfs v0.4.15.
    (I was able to reproduce the problem with ephemeral dockerized instance run via docker run --rm -it -p 8080:8080 -p 4001:4001 -p 127.0.0.1:5001:5001 ipfs/go-ipfs:v0.4.15)
  3. Try to upload the three files from step 1 (all at once, select them with Ctrl) via IPFS Companion's Quick Upload screen, you should get the error below:
    screenshot_57

How to fix

Not sure yet, but there are two bugs at play:

  1. go-ipfs returned http: invalid Read on closed Body for some reason
  2. ipfs-api ignored the error and returned a partial response (2 instead of 4 hashes) as a valid result

For now, Companion checks if result list has a proper count and displays an error if things are fishy, but it should be addressed upstream.

I am parking it for now and will pick up in spare time.
Until then, if anyone has any quick ideas, feel free to update this issue with any new findings.

@lidel lidel added kind/bug A bug in existing code (including security flaws) help wanted Seeking public contribution on this issue status/blocked/upstream-bug Blocked by upstream bugs status/ready Ready to be worked labels May 18, 2018
@lidel lidel changed the title ipfs.files.add broken due to "http: invalid Read on closed Body" in go-ipfs Unable to upload to IPFS API: passing multiple files to ipfs.files.add sometimes fails Jun 28, 2018
@lidel
Copy link
Member Author

lidel commented Jul 2, 2018

Upload via browser action fixed in #509 using workaround from ipfs/kubo#5168 (comment).
Landed in v2.4.0.10120 (Beta)

Upstream bugs can be tracked at: ipfs/kubo#5168 and https://github.com/ipfs/js-ipfs-api/issues/797

@lidel lidel closed this as completed Jul 2, 2018
@lidel lidel modified the milestone: v2.4.0 Jul 2, 2018
@lidel
Copy link
Member Author

lidel commented Jul 4, 2018

Reopening.

Chrome does not allow modification of Connection header in onBeforeSendHeaders and it remains at keep-alive, which means workaround from #509 won't work in Chrome:

Firefox

screenshot_5

Chrome

screenshot_36

@lidel lidel reopened this Jul 4, 2018
@lidel
Copy link
Member Author

lidel commented Aug 31, 2018

Good news!

ipfs/kubo#5168 (comment) provides another workaround: send /api/v0/add with Expect: 100-continue

And guess what: Chrome does allow setting Expect via onBeforeSendHeaders WebExtension API:

screenshot_23

This means we have a workaround that works for both Firefox AND Chrome.

Update: I checked Chrome docs and they do not include Expect on "restricted" list, but state this may change in future:

screenshot_24

So it is unclear if this is a feature, but its safe to use it for now and fix uploads for Chrome users.

Will PR shortly.

@lidel lidel closed this as completed in #562 Sep 3, 2018
lidel added a commit 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Seeking public contribution on this issue kind/bug A bug in existing code (including security flaws) status/blocked/upstream-bug Blocked by upstream bugs status/ready Ready to be worked
Projects
None yet
Development

No branches or pull requests

1 participant