Debug flaky TestStreamForServer test

[emcfarlane]

TestStreamForServer can fail on Send(nil) returning io.EOF. This occurs because Send(nil) will try to Write(nil) to the request body pipe. In almost all cases this will be read (and therefore succeed) when buffering the request body. However, on the server returning quickly, the request can be closed causing the Send to fail with io.EOF error.

To ensure a Send succeeds we need to match it with a correlating Receive in the server. Nil writes will never be guaranteed unless the server tries to Receive a message, which would read the request body and therefore enforce the successful drain. This is a problem for Send(nil) as the intended behaviour is to ensure the request headers are sent, which does complete but returns with an unexpected error.

To solve for Send(nil) we now check if this write caused the request to send and avoid triggering a body write if so. Any other cases and later if Send(nil) is called we Write(nil) which keeps the same behaviour.

Tests client-stream-conn and client-stream-send-msg write a message Send(msg). For these I've modified the test cases to Receive() the message to ensure that the write completes and doesn't otherwise affect the test case.

Fixes #626

[jhump]

TestStreamForServer can close the stream before reading the request body.

But why. Is this intentional?

If not, should we better synchronize the closing of the stream so it happens after the body has been read? If it is intentional, seems like we should instead just relax the test assertions to allow io.EOF from the call to Send.

To write a headers only request Send is called with zero length payload.

This is not accurate. It is called with nil. Any other zero-length payload actually sends a zero-length request message.

A remark on the linked issue:

Send(nil) is ignored if we get a close.

I don't understand the value of this. It seems to be solely for our test assertions, for which a superior fix is to relax the assertion. IMO, it seems perfectly reasonable that Send(nil) could return an error if it's expected that the connection might be closing/closed. Especially since this formulation only means "write headers only" if no messages have been sent; otherwise it is a no-op. So it seems odd to me that Send(nonNilMsg); Send(nil); Send(nonNilMessage) could conceivably result in a non-nil error, then a nil error(??). then a non-nil error.

[emcfarlane]

I'm not sure Send(nil) should be calling Write(nil) on the request body. The send ensures the request is created with client headers attached. I'd think the behaviour we would want from Send(nil) is to return nil if successfully creating the request and sending the headers. What should the behaviour be if multiple Send(nil) are called?

superior fix is to relax the assertion

Depends on the behaviour we expect from Send(nil). I thought returning an io.EOF would be unexpected.

[jhump]

Depends on the behaviour we expect from Send(nil). I thought returning an io.EOF would be unexpected.

Send is specified to return io.EOF if the stream has been closed, so the current behavior seems fine.

What should the behaviour be if multiple Send(nil) are called?

I think it's actually fine as is. Is there actually a problem with the current behavior? If calling code were organized in a way that multiple, independent layers of logic all called Send(nil), to ensure the request was started, are you suggesting that should that be an error? IMO, we don't need to special-case nil beyond the current implementation.

The bug in the flaky test sounds to me like an error is reasonable but the assertion is too strict.

BTW, I'm still curious: why can TestStreamForServer close the stream before reading the request body. Is that an intentional aspect of the test?

[emcfarlane]

why can TestStreamForServer close the stream before reading the request body

connect-go/connect_ext_test.go

Lines 1525 to 1532 in 20b5723

	client := newPingClient(&pluggablePingServer{
	cumSum: func(ctx context.Context, stream *connect.BidiStream[pingv1.CumSumRequest, pingv1.CumSumResponse]) error {
	return stream.Conn().Send("foobar")
	},
	})
	stream := client.CumSum(context.Background())
	assert.Nil(t, stream.Send(nil))
	_, err := stream.Receive()

The Send(nil) on line 1531 is never matched by a receive in the server handler. I think this almost always passes now because of buffering reads. But to ensure this nil write succeeds and can't be close by the handler exit I think you'd want:

func(ctx context.Context, stream *connect.BidiStream[pingv1.CumSumRequest, pingv1.CumSumResponse]) error {
	defer stream.Receive() // forces Read, get's io.EOF on Close from client
	return stream.Conn().Send("foobar")
}

This would force the request body to read the nil message, ensuring Send(nil) get a nil error. And because there's no Send(msg) the server Receive() will get an io.EOF when the client calls CloseRequest.

EDIT: fix code snippet go routine bug

[jhump]

I think you can undo all of the changes in this file, right? Looks like the addition of t here is unused, and this appears to be the only change in here.

[emcfarlane]

The servers close are matched to the clients which is nice. The other change in this file is the two Receives for the test cases that Send a non nil message.

[emcfarlane]

As in server := memhttptest.NewServer(t, mux) takes the correct subtests *testing.T rather than the parent.

[jhump]

Ah, okay. That was a bit subtle due to the shadowing of the name t.