Fix my own bug in postrun Execute

[djdv]

I left in a bad condition in #216 which was supposed to be deleted.
This should amend that and adds tests to make sure postruns actually executes if it should.

What was wrong?

The condition was supposed to be checking formatters, but was checking the wrong value.

Why is this condition removed instead of changed?

We're reading from a map type, even if it's nil we'll just get back the 0 value when indexing it.
In this case that's a nil function pointer, which is checked before being assigned and later used.
formatters[typer.Type()] != nil

[djdv]

@guseggert
Round 2. 😅

The testing code could probably be less copy-paste, but I think it at least works as expected.
I'm assuming it makes sense for Execute to block in this kind of setup, waiting for postrun to exit.
(and not blocking when nothing is trying to Emit)
^ A less roundabout way to do this might be to just return a defined error value from postRun and check for it.
Instead of trying to emit and receive something. This is probably better than spawning a goroutine to handle it, and should still be an alright way to make sure that postrun actually ran.
We could also do some closure magic too (set a value from postrun that was scoped outside of it), but that seems worse than both.

Not sure if there's a better way than using a mocked Typer to test this. And maybe it shouldn't use the CLI type rather than something custom. But all these are little nits.
We can make any changes if they're desired.

[djdv]

A less roundabout way to do this might be to just return a defined error value from postRun and check for it.

I went ahead with this. It simplifies the test considerably and should be equally as valid. We're testing the method not the emitter anyway.
Diff: https://github.com/ipfs/go-ipfs-cmds/compare/bba17015f8a4668a0ce2dc63a508fd7a53c3bf41..8998fa443d492177589218f243ab3deba9b2f2cc

[djdv]

For posterity I checked what commit I was using in my own repo and why it worked where the one submitted in the PR didn't.
It's not really important other than for the context of it.

diff --git a/executor.go b/executor.go
index 77f58d5..5bcfb7a 100644
--- a/executor.go
+++ b/executor.go
@@ -50,24 +50,24 @@ func (x *executor) Execute(req *Request, re ResponseEmitter, env Environment) er
 			return err
 		}
 	}
-	maybeStartPostRun := func(postMap PostRunMap) <-chan error {
+	maybeStartPostRun := func(formatters PostRunMap) <-chan error {
 		var (
 			postRun   func(Response, ResponseEmitter) error
 			postRunCh = make(chan error)
 		)
 
-		if cmd.PostRun == nil {
+		if postRun == nil {
 			close(postRunCh)
 			return postRunCh
 		}
 
-		// check if we have a function for this emitter
+		// check if we have a formatter for this emitter type
 		typer, isTyper := re.(interface {
 			Type() PostRunType
 		})
 		if isTyper &&
-			cmd.PostRun[typer.Type()] != nil {
-			postRun = cmd.PostRun[typer.Type()]
+			formatters[typer.Type()] != nil {
+			postRun = formatters[typer.Type()]
 		} else {
 			close(postRunCh)
 			return postRunCh

This falls in line with my assumption; I meant to check formatters but referenced the wrong variable when I changed maybeStartPostRun's signature.
Originally this was just a closure with no arguments, referencing cmd directly, so when I changed it to accept a PostRunMap as an argument, I should have dropped that check then instead of changing it. (like is done in this PR)

Edit: actually, that's not true. I see what's going on here and will update the commit.
The check was supposed to move below the assignment, which just eliminates reading from the map twice.

[djdv]

Good enough. 👌

[djdv]

This is interesting.
https://github.com/djdv/go-ipfs-cmds/runs/3558361879
I'm pretty sure my patch is not causing this failure. Running locally this test passes most, but not all the time.
I looked into it and, I think TestCancel may be implemented wrong, depending on randomness in the scheduler.

cancel should likely be called before the Emit goroutine even has the potential to run.
But making this change still did not fix the problem.
Within Emit, and Next for the chanResponseEmitter, it's also depending on randomness within select to choose the context error over sending the value, which is not guaranteed in any way.
So I just check up front if we're canceled, and bail out then. While leaving the selects which block on communication.
So if the caller never reads the value, but does call cancel at that point, they'll still get a context error also.

go test -v -race -run TestCancel -count 10000 would not pass for me before this on a Linux system, but now does.
So too does go test -v -race -count 10000 for the whole package on Linux and Windows.

I added the commit here but we can separate it out if desired.
b9ee0be

With the code being correct and CI passing, this should be ready for review for real now.

[djdv]

The declaration of ctx, and this check should probably be hoisted to the top of the function.
If we're canceled, we probably shouldn't do anything other than return.
The locking, EmitChan, etc. seems pointless if we'll get here anyway.

Need input/opinions on this.

[guseggert]

I'd rather just leave it next to the select like this, so that we preserve the existing post-cancellation side effects of consuming v if it's a chan, and unblocking Length(), which might be important somewhere else (don't know off the top of my head).

[djdv]

I think this makes sense, but that's because I wrote it.
If it could be reworded or is confusing, someone let me know.

[djdv]

cancel should likely be called before the Emit goroutine even has the potential to run.

Incorrect. This needs to go back where it was.
We pass either way, but at least when cancel is called late, there's a 50/50 chance we also test that pending sends will be canceled instead of sent.
Depending on who runs first, Emit or cancel.

[guseggert]

I'd rather just leave it next to the select like this, so that we preserve the existing post-cancellation side effects of consuming v if it's a chan, and unblocking Length(), which might be important somewhere else (don't know off the top of my head).