Improve streaming range responses

[kanongil]

This is a proper fix for #1777 so that it doesn't sometimes deliver 200 responses to range requests. This is achieved by simply not adding a content-length header to streaming responses. With this fix the response just contains less bytes than the content-range range would indicate, which is a valid response when the complete-length is unknown as indicated by /*.

FYI, returning less bytes than the content-range indicates is exploited in RFC 8673. Incidentally, this patch makes Varnish partially support such streaming range responses.

I have re-enabled the disabled tests from #1777, and revised all streaming range tests to expect 206 responses.

Background

I am using varnish to cache requests to a backend that delivers Low-Latency HLS. This includes a mode, where parts are delivered using byte range requests from the containing segments. Besides requiring byte-range support, this mode also requires the server to deliver data using streaming range responses. When I tried to deliver individual parts using byte range requests, Varnish failed to work and delivered a full segment in response to range requests (response code 200).

[dridi]

Very nice and tidy!

[kanongil]

FYI this new logic conflicts with the refactor in the pending #3859.

[dridi]

At quick glance it shouldn't be a conflict too hard to solve, thank you for pointing it out.

[dridi]

bugwash: we need to study the effect on HTTP/1 framing closely.

[kanongil]

Without a known resp_len, the byte stream will be transmitted using Transfer-Encoding: chunked.

The potentially new case is that the response processes less bytes than indicated by range_high. This case is handled here:

varnish-cache/bin/varnishd/cache/cache_range.c

Lines 51 to 64 in eec2a6b

	static int v_matchproto_(vdp_fini_f)
	vrg_range_fini(struct vdp_ctx *vdc, void **priv)
	{
	struct vrg_priv *vrg_priv;
	CHECK_OBJ_NOTNULL(vdc, VDP_CTX_MAGIC);
	CAST_OBJ_NOTNULL(vrg_priv, *priv, VRG_PRIV_MAGIC);
	if (vrg_priv->range_off < vrg_priv->range_high) {
	Req_Fail(vrg_priv->req, SC_RANGE_SHORT);
	vrg_priv->req->vdc->retval = -1;
	}
	*priv = NULL; /* struct on ws, no need to free */
	return (0);
	}

It probably needs to be patched so that the request doesn't fail when resp_len < 0, which I guess will cause the HTTP/1.1 connection to be closed.

[kanongil]

Amended my PR with a patch to not error on short ranges.

FYI, the logic in vrg_range_fini() is now only used when the backend response has a content-length header, as already tested in r02258.vtc. I moved the RANGE_SHORT log test to this file.

[dridi]

After a brief look at 535a5f0, I come to the same nice and tidy conclusion. You even went the extra mile and found a disabled test case that you rehabilitated. I missed this when I first reviewed this directly on Github and I just noticed after a first pass in my proper work environment.

FYI we have lined up 3 reviewers to comb this pull request.

[dridi]

It has taken me several partial attempts but I finally managed an almost complete review. I couldn't find anything wrong and I'm confident this is a solid change. We often get very good contributions but this one checks all my marks and then more.

Thank you again!

The last thing I need to do what I mentioned above in #3872 (comment) but I would rather adapt my work after merging this one.

[bsdphk]

Bugwash: OK.