Skip to content
This repository has been archived by the owner on Aug 1, 2023. It is now read-only.

go-ipfs ~~0.4.16~~ => 0.4.17 #28

Merged
merged 13 commits into from
Jul 28, 2018
Merged

go-ipfs ~~0.4.16~~ => 0.4.17 #28

merged 13 commits into from
Jul 28, 2018

Conversation

daviddias
Copy link
Member

I'm seeing a lot of failures with go-ipfs 0.4.16

Exchanging files

Seems that now exchanging files is prone to hang until timeout

  exchange files
    ✓ connect go <-> js (88ms)
    ✓ connect js <-> js
    cat file
      ✓ go -> js: 1.02 kB (270ms)
      ✓ js -> go: 1.02 kB (192ms)
      ✓ js -> js: 1.02 kB (410ms)
      ✓ go -> js: 63.5 kB (247ms)
      ✓ js -> go: 63.5 kB (185ms)
      ✓ js -> js: 63.5 kB (393ms)
      ✓ go -> js: 65.5 kB (248ms)
      ✓ js -> go: 65.5 kB (177ms)
      ✓ js -> js: 65.5 kB (391ms)
      ✓ go -> js: 524 kB (511ms)
      ✓ js -> go: 524 kB (331ms)
      ✓ js -> js: 524 kB (772ms)
      2) go -> js: 786 kB
      ✓ js -> go: 786 kB (3114ms)
      ✓ js -> js: 786 kB (853ms)
      ✓ go -> js: 1.05 MB (6304ms)
      ✓ js -> go: 1.05 MB (410ms)
      ✓ js -> js: 1.05 MB (855ms)
      3) go -> js: 1.05 MB
      ✓ js -> go: 1.05 MB (3850ms)
      ✓ js -> js: 1.05 MB (683ms)
      4) go -> js: 4.19 MB
      ✓ js -> go: 4.19 MB (4462ms)
      ✓ js -> js: 4.19 MB (1265ms)
      5) go -> js: 8.39 MB
      ✓ js -> go: 8.39 MB (1807ms)
      ✓ js -> js: 8.39 MB (1663ms)
    get directory
      6) go -> js: depth: 5, num: 5
      ✓ js -> go: depth: 5, num: 5 (335ms)
      ✓ js -> js: depth: 5, num: 5 (674ms)
      7) go -> js: depth: 5, num: 10
      ✓ js -> go: depth: 5, num: 10 (521ms)
      ✓ js -> js: depth: 5, num: 10 (673ms)
      ✓ go -> js: depth: 5, num: 50
      ✓ js -> go: depth: 5, num: 50 (1816ms)
      ✓ js -> js: depth: 5, num: 50 (1787ms)
      ✓ go -> js: depth: 5, num: 100
      ✓ js -> go: depth: 5, num: 100 (16902ms)
      ✓ js -> js: depth: 5, num: 100 (3386ms)

PubSub

Seems that the http-api response format for PubSub was changed as well

  11) pubsub
       ascii data
         publish from Go, subscribe on Go:
     Uncaught AssertionError: expected { Object (from, seqno, ...) } to have property 'from' of undefined, but got 'QmZMGA9d2Gw3BZUdNgDp49Twuk3uhzHyRz9inSEvKsA4YL'

@ghost ghost assigned daviddias Jul 15, 2018
@ghost ghost added the status/in-progress In progress label Jul 15, 2018
@daviddias
Copy link
Member Author

@Stebalien @whyrusleeping did you run the interop tests as part of the release or any of the RCs?

@richardschneider
Copy link

@diasdavid My C# HTTP API tests are all passing with go-ipfs 0.4.16. In fact a few tests are now working! It now handles

  • multiple subscribers to a pubsub topic
  • wrapping a file in a directory

@daviddias
Copy link
Member Author

Update, seems that PubSub is good. It just so happened that the exchange files test failures were interfering with the PubSub ones

@daviddias
Copy link
Member Author

I'm only seeing failures specifically on moving files from Go to JS, the other way around works 100% of the time.

Playing with the timeouts, I can see more of the tests go=>js passing, but the length has increased in a order of magnitude or more.

describe('repo', () => {
// The repo was again changed on go-ipfs 0.4.16
// TODO: create spec tests for repo
describe.skip('repo', () => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📍

@Stebalien
Copy link
Member

I did. As usual, they failed due to repo differences.

@Stebalien
Copy link
Member

Seems that the http-api response format for PubSub was changed as well

The issue there was in goD.api.id(cb). It should have returned an error but instead seemed to have returned an empty ID. Not sure if this is due to a commands lib issue in golang or the js-ipfs-api (but it looks like the latter). Thoughts?

@Stebalien
Copy link
Member

I'm only seeing failures specifically on moving files from Go to JS, the other way around works 100% of the time.

That's worrying. Is this new (not in go-ipfs 0.4.15)? See: ipfs/kubo#5183.

@Stebalien
Copy link
Member

So, I added a go -> go (two go daemons) test and that seems fine (really fast, actually). Definitely an interop issue.

@Stebalien
Copy link
Member

Found it: ipfs/js-ipfs#1445.

Basically, we're sending a block and then waiting for an EOF before sending the next block. There are two issues here:

  1. JS should close the stream.
  2. We should not be waiting. I implemented it that way for better back pressure but, really, that's not the right way to implement back pressure and will probably cause real-world performance issues.

@magik6k
Copy link
Member

magik6k commented Jul 18, 2018

Go to JS fix merged in ipfs/kubo#5258

@daviddias
Copy link
Member Author

daviddias commented Jul 19, 2018

Basically, we're sending a block and then waiting for an EOF before sending the next block.

I'm confused by the logic here. Are you saying you want a EOF (aka FIN packet in networks land) for each block you send out? If so, that breaks the streams abstraction as a stream that got closed/finalized should not be reused.

Go to JS fix merged in ipfs/kubo#5258

@magik6k any ETA for a release of that fix?

@Stebalien
Copy link
Member

I'm confused by the logic here. Are you saying you want a EOF (aka FIN packet in networks land) for each block you send out? If so, that breaks the streams abstraction as a stream that got closed/finalized should not be reused.

I'm saying that, when go sends a block and then closes the stream (for writing), it expects the other node to also close the stream (for writing) to completely close it. This was still was still a bug in go as there's no reason to wait for the EOF from JS before sending off the next block, but JS should eventually send that EOF.

@daviddias daviddias changed the title go-ipfs 0.4.16 go-ipfs ~~0.4.16~~ => 0.4.17 Jul 28, 2018
@daviddias
Copy link
Member Author

daviddias commented Jul 28, 2018

It's all good and shiny again!!! :D

  exchange files
    ✓ connect go <-> js (70ms)
    ✓ connect js <-> js (43ms)
    cat file
      ✓ go -> js: 1.02 kB (266ms)
      ✓ js -> go: 1.02 kB (179ms)
      ✓ js -> js: 1.02 kB (400ms)
      ✓ go -> js: 63.5 kB (258ms)
      ✓ js -> go: 63.5 kB (174ms)
      ✓ js -> js: 63.5 kB (382ms)
      ✓ go -> js: 65.5 kB (239ms)
      ✓ js -> go: 65.5 kB (160ms)
      ✓ js -> js: 65.5 kB (401ms)
      ✓ go -> js: 524 kB (515ms)
      ✓ js -> go: 524 kB (317ms)
      ✓ js -> js: 524 kB (760ms)
      ✓ go -> js: 786 kB (531ms)
      ✓ js -> go: 786 kB (354ms)
      ✓ js -> js: 786 kB (794ms)
      ✓ go -> js: 1.05 MB (566ms)
      ✓ js -> go: 1.05 MB (441ms)
      ✓ js -> js: 1.05 MB (794ms)
      ✓ go -> js: 1.05 MB (540ms)
      ✓ js -> go: 1.05 MB (415ms)
      ✓ js -> js: 1.05 MB (838ms)
      ✓ go -> js: 4.19 MB (867ms)
      ✓ js -> go: 4.19 MB (954ms)
      ✓ js -> js: 4.19 MB (1298ms)
      ✓ go -> js: 8.39 MB (20867ms)
      ✓ js -> go: 8.39 MB (1959ms)
      ✓ js -> js: 8.39 MB (2033ms)
      ✓ go -> js: 67.1 MB (6775ms)
      ✓ js -> go: 67.1 MB (26464ms)
      ✓ js -> js: 67.1 MB (11281ms)
      ✓ go -> js: 134 MB (41416ms)
      ✓ js -> go: 134 MB (28170ms)
      ✓ js -> js: 134 MB (21916ms)

Note the numbers. go-> js is 2x of js-> go and js->js.

image

@daviddias daviddias merged commit 6593ccd into master Jul 28, 2018
@daviddias daviddias deleted the go0.4.16 branch July 28, 2018 12:53
@ghost ghost removed the status/in-progress In progress label Jul 28, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants