server: return better status for context err when writing header

[idiamond-stripe]

I think this closes #4696

It looks like Write unconditionally returns status.Errorf(codes.Internal, "transport: %v", err) when we see an error writing the headers. However, in certain cases we'll actually have an overriding context error here, like a cancellation. This PR changes the behavior to return that instead if it's present.

This matches the behavior in the other branch of the conditional (

grpc-go/internal/transport/http2_server.go

Line 1028 in 1f12bf4

return ContextErr(s.ctx.Err())

), where if the stream is in state streamDone, this either returns ErrConnClosing or the s.ctx.Err(). We already check for the stream being streamDone inside WriteHeader, so I opted not to duplicate that check and instead just return the ctx error if it's present.

If we wanted a more specific fix, this could change

if s.ctx.Err() != nil {
    return ContextErr(s.ctx.Err())
}

to

if s.getState() == streamDone && s.ctx.Err() != nil {
    return ContextErr(s.ctx.Err())
}

but it duplicates the check inside WriteHeader.

(This is my first gRPC contribution, so apologies if I haven't followed the right process or I've overlooked something)

RELEASE NOTES:

• server: return Canceled or DeadlineExceeded status code when writing headers to a stream that is already closed

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: idiamond-stripe (4811f7a)

[dfawley]

Please update this text since we're more precise about sending this error now. E.g. "stream.SendHeader called multiple times". (It's SendHeader at the user API level.)

[dfawley]

GH won't let me comment on the right line, but on line 981, there's no guarantee this is a status error. I think it's always ErrConnClosing, in fact. Let's make that return status.Convert(err).Err() -- it's not really efficient or ideal, but it's only in an error condition so it should be okay. This means a closed connection will result in UNKNOWN, but since I'm pretty sure using statuses at all here is a bad idea in the first place, that should be fine. I think I'll separately change all the documentation on ServerStream to recommend against relying upon the returned errors' status codes.

[idiamond-stripe]

Are you suggesting changing the error inside writeHeaderLocked to be status.Convert(err).Err() or changing the handling of the return value to be status.Convert(err).Err()?

I changed the outside error, as that's the place where we converted it to status.Internal previously. Let me know if I understood wrong.

[dfawley]

Either way is probably fine. There is only one other usage of writeHeaderLocked, in WriteStatus, and the ultimate user of that just logs the result.

[dfawley]

There's a small window where the state can be streamDone but the context hasn't been canceled yet. In these cases, we would be returning an INTERNAL error.

We could paper over this race by calling s.cancel() here, as we seem to do in Write. But honestly I think the right fix here is to move s.cancel() in deleteStream to the first line of both finishStream and cancelStream. And then we can remove the s.cancel() in Write. Would you mind doing this and hopefully (:crossed_fingers:) all the tests still pass?

Looking closer at Write, it looks like it has special logic to prefer returning an error that the whole connection was closed over the stream being closed. We probably want to do the same thing here. If you want to copy/paste the select, that's fine. Or if you don't mind refactoring, it looks like we need:

func (t *http2Server) streamContextErr(s *Stream) error {
	select {
	case <-t.done:
		return ErrConnClosing
	default:
	}
	return ContextErr(s.ctx.Err())
}

But I can do that as a follow-on later otherwise.

[idiamond-stripe]

Yup, tests seem to all pass with this! I changed it finishStream, but it looks like it's already called in deleteStream (which closeStream calls).

[dfawley]

It is done by deleteStream, but there is a small race, still, where the context is not canceled while the state is streamDone. So I was suggesting moving that cancelation to happen before setting the stream's state to streamDone, and also removing it from deleteStream.

[idiamond-stripe]

Also is it possible to run the tests here? I'm a first time contributor so I think running the CI workflow requires signoff.

[dfawley]

Thanks! I think everything looks good except for moving the s.cancel call to closeStream directly, and out of deleteStream.

[dfawley]

Either way is probably fine. There is only one other usage of writeHeaderLocked, in WriteStatus, and the ultimate user of that just logs the result.

[dfawley]

It is done by deleteStream, but there is a small race, still, where the context is not canceled while the state is streamDone. So I was suggesting moving that cancelation to happen before setting the stream's state to streamDone, and also removing it from deleteStream.

[dfawley]

I think this can be deleted now.

[idiamond-stripe]

The TODO or the entire conditional? The tests appear to pass with the entire conditional removed but I'm not familiar enough with this code to understand whether we still need to check if the stream is done elsewhere in Write (if it's called after writing headers).

[idiamond-stripe]

It looks like writeQueue.get checks whether the writeQueue is done. Is the idea that that channel is set when the stream is closed?

[dfawley]

I was just referring to the TODO. I don't think this whole thing should be removed or else we'll potentially write operations into the control buffer for closed streams. If the controlbuf isn't checking that the stream is still valid and writes frames, then that's an HTTP/2 protocol violation.

[idiamond-stripe]

ack. reverted and removed the TODO.

[menghanl]

LGTM