H2 GOAWAY review

[nigoroll]

In the context of #4284 I read rfc9113 again, cross-checking with our code, and I now think that our GOAWAY handling is incomplete to wrong.

We have a single goaway flag

varnish-cache/bin/varnishd/http2/cache_http2.h

Line 174 in 440319d

int goaway;

Yet the semantics are different:

Sending it means "I will ignore any new streams", and, because we do not implement PUSH, we are the only endpoint which could want this semantic. But a client is also free to use it to signal the intend of a graceful shutdown: a client GOAWAY does not mean to abort everything, it means "finish current streams". If a client wanted to get rid of open streams, it would reset them.

Activity on streams numbered lower than or equal to the last stream identifier might still complete successfully. The sender of a GOAWAY frame might gracefully shut down a connection by sending a GOAWAY frame, maintaining the connection in an "open" state until all in-progress streams complete.

Once the flag is set, we never send a goaway

varnish-cache/bin/varnishd/http2/cache_http2_proto.c

Lines 421 to 422 in 440319d

	if (h2->goaway || !h2e->send_goaway)
	return;

See before: We would have good reason to send a goaway even after having received one

Once the flag is set, we stop receiving data

varnish-cache/bin/varnishd/http2/cache_http2_proto.c

Lines 1478 to 1479 in 440319d

	if (h2->goaway && h2->open_streams == 0)
	return (0);

We absolutely MUST not do this

However, any frames that alter connection state cannot be completely ignored. For instance, HEADERS, PUSH_PROMISE, and CONTINUATION frames MUST be minimally processed to ensure that the state maintained for field section compression is consistent (see Section 4.3); similarly, DATA frames MUST be counted toward the connection flow-control window. Failure to process these frames can cause flow control or field section compression state to become unsynchronized.

When we receive a goaway, we simply set the last stream id

varnish-cache/bin/varnishd/http2/cache_http2_proto.c

Lines 397 to 411 in 440319d

	static h2_error v_matchproto_(h2_rxframe_f)
	h2_rx_goaway(struct worker *wrk, struct h2_sess *h2, struct h2_req *r2)
	{
	CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
	ASSERT_RXTHR(h2);
	CHECK_OBJ_NOTNULL(r2, H2_REQ_MAGIC);
	assert(r2 == h2->req0);
	h2->goaway = 1;
	h2->goaway_last_stream = vbe32dec(h2->rxf_data);
	h2->error = h2_connectionerror(vbe32dec(h2->rxf_data + 4));
	H2S_Lock_VSLb(h2, SLT_Debug, "GOAWAY %s", h2->error->name);
	return (h2->error);
	}

but we must check the last stream id to be lower than any previous one

Endpoints MUST NOT increase the value they send in the last stream identifier, since the peers might already have retried unprocessed requests on another connection.

Side notes

We do not implement the "graceful shutdown" procedure

A server that is attempting to gracefully shut down a connection SHOULD send an initial GOAWAY frame with the last stream identifier set to 231-1 and a NO_ERROR code. This signals to the client that a shutdown is imminent and that initiating further requests is prohibited. After allowing time for any in-flight stream creation (at least one round-trip time), the server MAY send another GOAWAY frame with an updated last stream identifier. This ensures that a connection can be cleanly shut down without losing requests.

We do not do this because we do not currently implement the "graceful shutdown"