commands: expected type <-chan interface{}, got <nil>

[Stebalien]

When sending a SIGINT to ipfs repo verify, I occasionally get:

128 blocks processed.d.
Error: expected type <-chan interface {}, got <nil>

This was originally reported at #4360 (comment) (and believed fixed). However, the issue itself was a separate problem so I'm opening a new issue.

[Stebalien]

@kevina

[keks]

I'm having trouble reproducing this. What happens here is that sometimes it doesn't print an error. The -D flag isn't helpful either.

[Stebalien]

I can reproduce it about 50% of the time. You need to let the command run for a second before hitting ^C. Try adding a bunch of large files so ipfs repo verify takes longer.

[schomatis]

@Stebalien Is someone currently working on this? (I'm checking because I saw this issue in the release blockers list.)

There seems to be a race condition between the goroutine of the ipfs repo verify and the goroutine that emits/marshals the output. The marshaler is receiving a nil output because the context was canceled (SIGINT/CTRL+C) while the emitter is still reading values from the output channel to marshal.

[Stebalien]

@schomatis thanks for diagnosing this! Nobody's currently working on it, feel free.

I took one look, gave up, and migrated this command to the new commands lib (#4764). However, having a proper fix would be really nice.

[schomatis]

@Stebalien This seems more a problem of the legacy commands library than the ipfs repo verify command (e.g., ipfs filestore dups and ipfs filestore verify have the same error). The Output function of responseWrapper discards the error received from Next so when the output returned is nil there is no indication if this is expected (command canceled, stop printing output) or if there was an error. The code implementing the command can explicitly check if it has been canceled (see PoC #4773), but I'm not sure if this should be the responsibility of the command.

In case the fix should happen inside the commands library (and I'm inclined to think so) I'm going to need more guidance as to where it should be placed.

[Stebalien]

I see what the problem is now.

So, when we switched to the new cmds library, we appear to have partially migrated marshalers. We used to call the marshaler once for the entire stream. We now call it for each item. However, the interface still acts like we're calling it once. This makes it really error prone, hacky, and shitty. I'm going to try some sed magic to fix this.

[Stebalien]

So, this really just sucks all around. I think the best way to fix this is to:

1. Manually migrate commands with multiple outputs to the new system.
2. Sed-migrate everything else to an intermediate.

I really have no idea how I missed this in review.

[Stebalien]

All done.