Fix exec deadlock when emitter is not Typer intf

[djdv]

There's probably a better way this could be done but I don't have time to investigate it. Edits welcomed.

If the emitter passed to the executor does not implement the Type method, it will just block forever on the channel operations to postCloseErr.

Example:

package main

import (
	"context"
	"io"
	"log"

	cmds "github.com/ipfs/go-ipfs-cmds"
)

func main() {
	var (
		testCmd = &cmds.Command{
			Run: func(*cmds.Request, cmds.ResponseEmitter, cmds.Environment) error { return nil },
			PostRun: cmds.PostRunMap{
				cmds.CLI: func(response cmds.Response, emitter cmds.ResponseEmitter) error { return nil },
			},
		}
		testRoot = &cmds.Command{
			Subcommands: map[string]*cmds.Command{
				"test": testCmd,
			},
		}
		exec = cmds.NewExecutor(testRoot)
	)

	request, err := cmds.NewRequest(context.Background(), []string{"test"}, nil, nil, nil, testRoot)
	if err != nil {
		log.Fatal(err)
	}

	emitter, response := cmds.NewChanResponsePair(request)

	mockEmitter := &mock{emitter}

	// Does not deadlock.
	exec.Execute(request, mockEmitter, nil)
	if _, err := response.Next(); err != io.EOF {
		log.Fatal(err)
	} else {
		log.Println("okay")
	}

	// Deadlocks because emitter.(ChanResponse) does not implement .Type()
	exec.Execute(request, emitter, nil)
}

type mock struct {
	cmds.ResponseEmitter
}

func (*mock) Type() cmds.PostRunType { return cmds.CLI }

I encountered this a while ago when trying to write tests using the cmds.NewChanResponsePair structures.
In those scenarios they would just never return. When isolated like this, Go flatout panics.

go run .\main.go
2021/08/22 09:13:50 okay
fatal error: all goroutines are asleep - deadlock!

goroutine 1 [chan receive]:
github.com/ipfs/go-ipfs-cmds.(*executor).Execute(0xc0000cdf60, 0xc000124000, {0xf1cd48, 0xc0000d60f0}, {0x0, 0x0})
        C:/Users/Dominic Della Valle/Projects/Go/pkg/mod/github.com/ipfs/[email protected]/executor.go:78 +0x3c5
main.main()
        C:/Users/Dominic Della Valle/AppData/Local/Temp/c/main.go:45 +0x41e

goroutine 19 [select]:
github.com/ipfs/go-log/writer.(*MirrorWriter).logRoutine(0xc0000dd890)
        C:/Users/Dominic Della Valle/Projects/Go/pkg/mod/github.com/ipfs/[email protected]/writer/writer.go:71 +0x110
created by github.com/ipfs/go-log/writer.NewMirrorWriter
        C:/Users/Dominic Della Valle/Projects/Go/pkg/mod/github.com/ipfs/[email protected]/writer/writer.go:36 +0xcf
exit status 2

NewChanResponsePair should probably implement Type, but this still needs to be fixed in case someone passes any other type to Execute.

* I think that's all right, I'm recalling from months ago, but some details may be wrong. Just poke me for details.

[guseggert]

LGTM, I added a test to cover this as well. Thanks!

[djdv]

@guseggert
Sorry about this, but this patch was no good.

I just noticed now that the nil check for postRun will always be true because the declaration is right above it.
I'm pretty sure this causes PostRun to never be called at all.

I think this check was originally supposed to check the input argument formatters postRunMap, not postRun. I must have renamed the argument from postRun to formatters at some point without updating the name in the condition.
Or if I did, likely only updated my vendored copy, not the one in the branch.

I'm going to investigate this now, and will likely make a follow up PR.
My bad, I wanted to look it over again sooner but didn't have the time.

[guseggert]

Sounds good, thanks for the follow-up! Would you mind adding whatever tests/assertions are missing that would have caught that?

[djdv]

Would you mind adding whatever tests/assertions are missing that would have caught that?

For sure. I was surprised the existing ones didn't catch it.
I'm at least adding one which emits something from PostRun, and will consider if there's any other scenarios I can tack on.

Current draft is looking like this (but I need to mock responder so that it pretends to be the CLI or something else):

func TestExecutorPostRun(t *testing.T) {
	expectedValue := true
	testCmd := &Command{
		Run: func(*Request, ResponseEmitter, Environment) error {
			return nil
		},
		PostRun: PostRunMap{
			CLI: func(response Response, emitter ResponseEmitter) error {
				return emitter.Emit(expectedValue)
			},
		},
	}
	testRoot := &Command{
		Subcommands: map[string]*Command{
			"test": testCmd,
		},
	}
	req, err := NewRequest(context.Background(), []string{"test"}, nil, nil, nil, testRoot)
	if err != nil {
		t.Fatal(err)
	}

	t.Run("with", func(t *testing.T) {
		var (
			emitter, resp = NewChanResponsePair(req)
			x             = NewExecutor(testRoot)
		)
		err = x.Execute(req, emitter, nil)
		if err != nil {
			t.Fatal(err)
		}

		response, err := resp.Next()
		if err != nil && err != io.EOF {
			t.Fatal(err)
		}
		responseTyped, isBool := response.(bool)
		if !isBool {
			t.Fatalf("expected postrun response to be `bool` but got: %#v",
				response)
		}
		if responseTyped != expectedValue {
			t.Fatalf("expected postrun to respond with %v but got: %v",
				expectedValue, responseTyped)
		}

		_, err = resp.Next()
		if err != io.EOF {
			t.Fatalf("expected EOF but got: %s", err)
		}
	})

	t.Run("without", func(t *testing.T) {
		testCmd.PostRun = nil

		var (
			emitter, resp = NewChanResponsePair(req)
			x             = NewExecutor(testRoot)
		)
		err = x.Execute(req, emitter, nil)
		if err != nil {
			t.Fatal(err)
		}

		_, err := resp.Next()
		if err != io.EOF {
			t.Fatalf("expected EOF but got: %s", err)
		}
	})
}