Testing a Go channel handoff without sleep

This moves the point where the operation returns. Today it returns once the handoff happens. The revised version returns after the acknowledgement. Anything written against the first one now sits through the worker's processing without having asked to.

That may well be the better promise. It is still a migration, and the existing callers pay for it. I'd keep both completion points in the API, each with its own name, so old code can stay put until it's ready to move. The old caller will still compile cleanly, since its types didn't change, only how long it waits.

Check the id on the acknowledgement too. A value shaped like a reply can still belong to some other request. In OCaml that's two names bound in the pattern and one comparison in the guard, nothing fancier.

What if the two promises just get two words? accepted for the handoff, completed for the result. wren's unchanged caller keeps waiting for acceptance and never notices a thing, and a caller that needs the result asks for completed.

Now play the awkward run. The worker reports acceptance, then exits before it ever reports completion. On the BEAM it's tempting to point at process isolation and call the thing resilient, but isolation doesn't come with a recovery policy, and a link can carry that exit to processes nobody had in mind. So completed needs a partner for the run where the worker dies first. Does the caller get a failure message, a retry, or just its own timeout? Whichever it is, write it down next to the name.

Run the sleep version under -race before retiring it. It passes plain and gets flagged with the detector on, because a sleep adds no happens-before edge.

Seconding kpw. A sleep gives no happens-before edge, so the plain run stays green and -race reports DATA RACE on the slot, worker write against test read.

The ack version holds for one request at a time and no further. There's a case I hadn't written down, two requests in flight before the test reads any acks.

var a, b int
in <- req{id: 1, slot: &a}
in <- req{id: 2, slot: &b}
<-ack
<-ack

That never finishes, -timeout has to end it. The worker takes request one, writes the slot, and blocks sending ack one, because nobody is receiving yet. The test is blocked sending request two, because the worker isn't receiving either. Each side waits on the other's channel. The race detector has nothing to say about it, since there's no race, just two goroutines parked. A plain go test hangs the same way, so this one isn't hiding behind the detector at all.

So "one channel each way" is fine for the single-request test, with the id check on the ack as well, it costs one comparison. For more than one in flight I'll need either a buffer on ack or a reply route per request, and I'm not sure yet which one keeps the test as easy to read. Whatever I pick, the two-request case goes into the test file so it can't come back unnoticed.

The count is easy to state. With capacity k on ack, the test can send k + 1 requests before it has to read one. Unbuffered gives you one, which is the hang. A buffer of two lets three through and parks the fourth. Fine for a test that knows its count. For anything else it's a limit you have to keep in step with the caller.

Neither. The ack channel is the problem, not its size. Give each request its own done channel and have the worker close it after the write.

type req struct {
	id   int
	slot *int
	done chan struct{}
}

func worker(in <-chan req) {
	for r := range in {
		*r.slot = r.id
		close(r.done)
	}
}

Close never blocks, so two in flight or twenty, the worker never waits on the test. A buffer only moves the hang to a bigger number. The shared ack reads easier with one request, sure, but that test already exists.

Before this gets adopted, look at callers that build req by field name. They don't break at compile time when done is added. Their literals keep compiling with a nil done, and the worker's close panics on the first request. An unkeyed literal would at least fail to build. I'd put a constructor in front of the struct before anyone gets used to writing it out.

That's nicer than either of mine. It also makes the id check pointless, since a request can only ever see its own channel close. I'd still keep a timeout around the receive, for the same reason as before.

Fine by me. The id check only existed because one channel carried every reply, and per-request channels remove that mix-up.

Fair. Export a NewRequest(id, slot) and keep the fields unexported, so outside callers can't skip done.

Leak. Nothing closes in, so worker sits in range in after the test returns, one parked goroutine per run. defer close(in) right after the make. Kotlin's coroutineScope wouldn't even let the test return with that child alive.

Fair, and it fits nicely now that the worker only ever closes channels. Here's where the test ended up, with tam_ly's done and your defer.

func TestTwoInFlight(t *testing.T) {
	in := make(chan req)
	defer close(in)
	go worker(in)
	var a, b int
	r1, r2 := newReq(1, &a), newReq(2, &b)
	in <- r1
	in <- r2
	for _, r := range []req{r1, r2} {
		select {
		case <-r.done:
		case <-time.After(2 * time.Second):
			t.Fatal("stuck")
		}
	}
	if a != 1 || b != 2 {
		t.Fatalf("slots %d %d", a, b)
	}
}

It passes with -race -count=3. The two-request case that hung before now finishes, because the worker never waits on the test anymore. Closing in also ends the worker the plain way, range sees the closed channel and returns. newReq stays unexported for now. It's a helper in the same package, and nothing outside builds a req yet.

Before choosing between a buffer and a reply route, read what -timeout printed. A test binary that times out panics and dumps every goroutine. For the two-request version the dump shows the test goroutine in [chan send] on the line of the second in <-, and the worker in [chan send] on its ack <- line. That is the complete diagnosis, with line numbers.

Run it with -timeout 3s so you are not waiting out the default ten minutes. Then choose the fix that removes one of those two sends, and confirm it with the same dump.

Could the done channel carry the result too? Something like make(chan int, 1) per request. The worker sends r.id once, and the test receives the value instead of reading a slot. With a buffer of one the worker's send never waits, so it keeps what tam_ly was after, and the pointer goes away. Maybe that's simpler to read, though I can see it changing what the test is about.

If outside callers only ever wait on it, hand the channel out receive-only.

func (r Request) Done() <-chan struct{} { return r.done }

Then close(r.Done()) in a caller doesn't build, the compiler reports that it cannot close a receive-only channel. Only code inside the package sees the field with its full type, which in practice means the worker.

Keep the constructor, but it does not make a bad request impossible. From another package, var r work.Request and work.Request{} both compile with every field unexported. Either one gives the worker a nil done, and close panics with "close of nil channel". A C# private constructor rules that out. Go has no way to forbid the zero value.

So the worker should check r.done == nil and skip the request with a log line. One malformed caller then cannot take the worker down.

The receive-only view settles who may close the channel. It does not cover the same request being handed to the worker twice. A caller that resubmits after its own timeout sends a Request whose done the worker may already have closed, and the second close panics with "close of closed channel".

The rule therefore has two parts. The worker closes done exactly once, and each request value is submitted only once. The type expresses the first part. The second is an obligation on the caller that no type here can state, so it should be written in the doc comment on NewRequest.

Keep the sleep version as an exercise before deleting it. Make the sleep longer and run -race again, it still reports the race, and that's usually when it clicks that the detector isn't timing anything.

Worth counting what "twenty in flight" means with one worker and an unbuffered in. Send one completes when the worker takes request one. Send two can't complete until the worker has written slot one, closed done one and come back around to range. So when the test's twentieth send returns, nineteen requests are finished and one is being handled.

Nothing deadlocks, but nothing overlaps either. If the test is meant to show requests waiting at the same time, it needs a buffered in or a second worker.

I thought about that, and I'd rather keep the slot. The thing under test is that a write into memory the caller handed over is visible once done closes. The worker this test is for fills in a slot it was given, it doesn't send results back. If the value travels on the channel, the test checks the channel, which Go already guarantees, and the write we care about drops out of the picture.

For a worker that does return values, your version is the nicer one.

Carry the zero-value caller through that change, though. Today it panics on the first request, loudly, with a stack that points at close. With the nil check the worker skips it, and that caller then waits on r.Done(), which is a receive from a nil channel, and that blocks forever. The old caller goes from a crash to a hang, and a hang in a test is what this thread set out to remove.

I'd keep the panic and give it a better message, one that names NewRequest.