Dag export command, complete

[ribasushi]

This is the most non-controversial part of all. The commented-out go-ipld-prime hickup is being debugged by @warpfork and me over at https://github.com/ribasushi/gip-muddle-up

[Stebalien]

We shouldn't need to call this.

[ribasushi]

done

[Stebalien]

Ah, wait, actually... we need to call pipeR.Close() to unblock the writer, I think (it can't hurt to call it anyways).

[ribasushi]

I modeled original code on the example at https://golang.org/pkg/io/#Pipe is showing a writer close only, and no reader closures. That's also consistent with "posix-ish thinking". Are you sure you want to move to pipeR.Close() instead?

[ribasushi]

I just did it anyway, even though it doesn't seem right... Closing a reader will just block a writer, though these are virtual writers so shouldn't matter.

[Stebalien]

Closing the reader will unblock the writer: https://golang.org/pkg/io/#PipeReader.Close. I'm concerned about the following:

1. Copying from the pipe to the http writer fails because the http connection dies.
2. Emit returns an error.
3. We walk away but the CAR writer is still trying to write to the pipe.

Closing the writer will unblock the CAR writer and cause it to abort with an error.

[ribasushi]

Oh... ok... that makes sense... BUT
Then the code can be made much safer like:

diff --git a/core/commands/dag/dag.go b/core/commands/dag/dag.go
index 6835d0945..ec4e88c94 100644
--- a/core/commands/dag/dag.go
+++ b/core/commands/dag/dag.go
@@ -295,6 +295,7 @@ The output of blocks happens in strict DAG-traversal, first-seen, order.
                // if err := car.Write(pipeW); err != nil {}
 
                pipeR, pipeW := io.Pipe()
+               defer pipeR.Close() // ignore the error if any, ublock the writer goroutine to exit
 
                errCh := make(chan error, 2) // we only report the 1st error
                go func() {
@@ -319,7 +320,6 @@ The output of blocks happens in strict DAG-traversal, first-seen, order.
                }()
 
                if err := res.Emit(pipeR); err != nil {
-                       pipeR.Close() // ignore the error if any
                        return err
                }
 

Not pushing it now not to muddle things up, but if you agree with add it to the import rebase

[Stebalien]

You're right. That's much better.

[ribasushi]

Everything is terrible. THIS is the correct way to close the pipe: 70834450926f53902

This is the only extra commit I added since your LGTM, the rest is just merging master. Not planning to push more commits here unless the above closer needs fixing.

[ribasushi]

Ready for final re-review ( every comment addressed, sharness fixed )

[Stebalien]

I missed a few things and I'd like to avoid making any assumptions about when the fixes to go-ipld-prime will ship.

[ribasushi]

no worries, I am going down the list already, fixes to be pushed in ~15

[Stebalien]

Provisional LGTM pending tests.

[ribasushi]

@Stebalien this currently is exactly at where you said "LGTM" + the concurrency-free progressbar. If this passes sharness - it should be good to merge as-is.

I am working on the import part based off ccbd08b16f96

[ribasushi]

And of course it fails tests, seemingly unrelated:

=== FAIL: core/coreapi/test TestIface/Dht (1.37s)
2020-03-28T05:50:04.000Z	ERROR	core	core/builder.go:47	failure on stop: context canceled
github.com/ipfs/go-ipfs/core.NewNode.func2
	/home/circleci/ipfs/go-ipfs/core/builder.go:47
    --- FAIL: TestIface/Dht (1.37s)

[Stebalien]

That's a known flaky test, unfortunately.

[ribasushi]

@Stebalien latest push on this branch merges master one last time, addresses #7036 (comment), and removes urfave/cli.

[Stebalien]

Rebased to remove merge commits.