use go's built in handling of trailers and dont do custom chunking

[whyrusleeping]

It turns out they implemented http trailers in go's std http lib. I am happy.

License: MIT Signed-off-by: Jeromy jeromyj@gmail.com

[tv42]

Note: 1.5 only.

[whyrusleeping]

Note: 1.5 only.

worth it.

[whyrusleeping]

this also allows us to revive this PR: https://github.com/ipfs/go-ipfs/pull/1388

[jbenet]

People have not all migrated to go 1.5. unlike the go 1.3 -> go 1.4 change, the go 1.5 change introduces major runtime differences, and possible problems. Everyone's still getting adjusted + still finding problems. I do not want to require go 1.5 yet.

[whyrusleeping]

@jbenet but if we're providing prebuilt binaries, does it matter?

[whyrusleeping]

@jbenet the api is also pretty broken in general without this. try catting a file larger than a few mbs

[tv42]

Also, Hijack -> connection is closed after every request -> client that does more than one request can see spurious errors. Alternative fixes on the client side aren't pretty: 1) have a retry loop OR 2) set http.Request.Close = true for all requests OR 3) wait for a version of Go with https://github.com/golang/go/issues/4677 in stdlib.

[jbenet]

• https://news.ycombinator.com/item?id=10087227

@jbenet the api is also pretty broken in general without this. try catting a file larger than a few mbs

not seeing the problem?

i agree we want to fix this and this is the way to fix it. i dont yet want to break go 1.4 compat. lots of people build from source who have not upgraded. myself included.

[whyrusleeping]

@jbenet https://github.com/ipfs/go-ipfs/issues/1620

[whyrusleeping]

I guess we can just sit on this PR until we're ready to jump to go1.5. I will be pressing for the 1.5 switch pretty heavily though, it will fix a large number of the random little api issues we see, as well as allow us to use the close notify stuff finally

[thelinuxkid]

ipfs/starship's dkr-shp can use this for tar and user image names, e.g., jbenet/go-ipfs. Tested to work.

[whyrusleeping]

it appears that dkr-shp doesnt work without this.

[jbenet]

it worked for me, without this. i havent tested latest commit

[thelinuxkid]

It worked for me only when using single-word-image-names. Which name did you use?

[jbenet]

i was using the the image id

[thelinuxkid]

Got it. The issue is with user namespaces (which translate to paths in ipfs) and for some reason hyphens. The following did not work for me: dkr-shp pull go-ipfs Error: http: unexpected EOF reading trailer dkr-shp pull jbenet/goipfs Error: http: unexpected EOF reading trailer dkr-shp pull jbenet/go-ipfs Error: http: unexpected EOF reading trailer

Additionally, ifps tar add returns: Error: write /dev/stdout: broken pipe

[thelinuxkid]

Can we get gobuilder to work with this branch? If so, it's just a matter of changing the version in starship...for now.

[jbenet]

yeah i think we can just ship a tag, gobuilder builds tags. the tar works for me, i was seeing the broken pipe stuff too and decided it was docker (it did it on outputting to | less too)

[Mithgol]

This comment is written for this thread to appear in my https://github.com/notifications/participating instead of usual notifications.

[whyrusleeping]

@jbenet we want this to land in 0.3.8.

[jbenet]

@whyrusleeping ok. but first:

• [x] rebase

what happens if you try to compile Go 1.5.1 only code with Go 1.4? is there a good, meaningful error that says "this requires Go 1.5.1, please install it"? (i doubt it) if not:

• [x] add a note to readme that this requires Go 1.5.1
• [x] add a check to the makefile that tells the user to get Go 1.5.1 if user tries to compile with <1.5.1

[tv42]

To require Go 1.5, you can put in a new file with a build tag and an intentional error:

require_go15.go:

// +build !go1.5

`IPFS needs to be built with Go version 1.5 or greater`

$ go version
go version go1.5.1 linux/amd64
$ go build
$ go14 version
go version go1.4.3 linux/amd64
$ go14 build
require_go15.go:3:1: expected 'package', found 'STRING' `IPFS needs to be built with Go version 1.5 or greater`

[mappum]

Does the golang stdlib now allow writing the response body before the client has finished their request body? It didn't before, which was why we had to resort to the initial custom chunk writer in the first place.

[whyrusleeping]

@mappum I think so. I mean, nothing breaks... Was there a certain case you remember?

[mappum]

Was there a certain case you remember?

Yes, if you can add a file and have the daemon stream the progress output (ipfs add -p) then it is working correctly.

[jbenet]

@mappum @whyrusleeping there should be a sharness test for it, before this merges.

[whyrusleeping]

verified this works locally. No idea how to write a sharness test for this, since the condition was 'streams output back while the file is being streamed to the server'

[jbenet]

No idea how to write a sharness test for this, since the condition was 'streams output back while the file is being streamed to the server'

There would be output coming from a curl.

[whyrusleeping]

@jbenet the problem that @mappum describes will be nearly impossible to write a sharness test for. I am confident that it works as expected.

[jbenet]

Nearly impossible? Write a go tool that uses http pkg and use it from sharness. Straight forward.

This problem has been plaguing users. We need to test it. Manual testing will not happen on every single commit going forward

— Sent from Mailbox

On Mon, Oct 12, 2015 at 5:04 AM, Jeromy Johnson notifications@github.com wrote:

@jbenet the problem that @mappum describes will be nearly impossible to write a sharness test for. I am confident that it works as expected.

Reply to this email directly or view it on GitHub: https://github.com/ipfs/go-ipfs/pull/1621#issuecomment-147245896

[whyrusleeping]

Nearly impossible?

referring to doing it in shell. But yeah, i guess doing it in go might be a little easier...

[whyrusleeping]

@jbenet to be clear, the test youre wanting to see is one that tests that the progress bar can be sent back before the request body sent by the client is complete?

[jbenet]

The test needed is something that would fail without this change (ie reproduces the problem before-- if it's intermittent run it many times back to back until it fails) but succeeds with this change. If the test you describe captures that, so be it, but I think it's more about the format screwing up, no?

— Sent from Mailbox

On Mon, Oct 12, 2015 at 12:37 PM, Jeromy Johnson notifications@github.com wrote:

@jbenet to be clear, the test youre wanting to see is one that tests that the progress bar can be sent back before the request body sent by the client is complete?

Reply to this email directly or view it on GitHub: https://github.com/ipfs/go-ipfs/pull/1621#issuecomment-147290321

[whyrusleeping]

ooooookay. we were on two separate trains of thought again.

[whyrusleeping]

alright, so the only place i've been able to somewhat reliably reproduce the issue is in the t0130-multinode.sh test in fix/bitswap-hang. Without this branch the first cat fails like so:

whyrusleeping@idril ~/g/s/g/i/g/t/sharness (fix/bitswap-hang)> while true
                                                                   ./t0130-multinode.sh -v
                                                               end
expecting success: 
    iptb init -n 5 -p 0 -f --bootstrap=none

ok 1 - set up tcp testbed

expecting success: 
        iptb start

Started daemon 0, pid = 31019
Started daemon 1, pid = 31028
Started daemon 2, pid = 31036
Started daemon 3, pid = 31044
Started daemon 4, pid = 31052
ok 2 - start up nodes

expecting success: 
        iptb connect [1-4] 0

connecting 1 -> 0
connecting 2 -> 0
connecting 3 -> 0
connecting 4 -> 0
ok 3 - connect nodes to eachother

expecting success: 
        check_has_connection 0 &&
        check_has_connection 1 &&
        check_has_connection 2 &&
        check_has_connection 3 &&
        check_has_connection 4

ok 4 - nodes are connected

expecting success: 
        random 1000000 > filea &&
        FILEA_HASH=$(ipfsi 1 add -q filea)

ok 5 - add a file on node1

expecting success: 
        ipfsi $node cat $fhash > fetch_out

ok 6 - can fetch file

expecting success: 
        test_cmp $fname fetch_out

ok 7 - file looks good

expecting success: 
        ipfsi $node cat $fhash > fetch_out

ok 8 - can fetch file

expecting success: 
        test_cmp $fname fetch_out

ok 9 - file looks good

expecting success: 
        ipfsi $node cat $fhash > fetch_out

Error: read tcp 127.0.0.1:38808->127.0.0.1:33008: read: connection reset by peer
not ok 10 - can fetch file
#   
#           ipfsi $node cat $fhash > fetch_out
#       

[whyrusleeping]

alright, so it looks like t0130-multinode.sh fails about 1 in 5 times on my laptop due to the bug this PR fixes. I'm running the test over and over again on this branch to make sure it doesnt reappear.

[whyrusleeping]

alright, up to 100 runs of that test. no failures so far. gonna leave it going for a little while just to be sure.

[whyrusleeping]

alright, that ran for a while, no failures at all.

[jbenet]

@whyrusleeping so it fails in prior commits, but not this one? If so, how about we add it, and run the critical part N times (for an N that runs < 5s on avg). but let's make sure it does fail prior, and succeed now.

[jbenet]

(it's fine if <5s is not enough for triggering the failure, because as it is run tons of times, twice per PR, we'll likely see the failure).

[whyrusleeping]

the test is already added. it merged in the bitswap-fix PR (right before this one). Do you want to change the test to run multiple times?

[whyrusleeping]

I have run the test multiple times before the PR and it does indeed fail, as shown in https://github.com/ipfs/go-ipfs/pull/1621#issuecomment-147298024

[jbenet]

@whyrusleeping ok i'll just merge this. ideally we would run it however many times it takes to make it a deterministic failure in the last commit (is that 1 time?).

[jbenet]

Warning to users who compile: from now on, go-ipfs will only build in Go 1.5.1 and later. Everyone should update their compiler.