Send VFP_END with last data for v1f chunked & read ahead #3809

nigoroll · 2022-05-28T17:09:17Z

While working on request body caching improvements, it was noticed that, for chunked encoding, we do not know the request body size before we have attempted to read at least one additional byte because the chunked fetch did not return VFP_END with the last chunk, but rather only as a zero-length additional chunk.

This commit changes chunked encoding processing to read the next chunk header before returning the last bytes of the previous chunk. This allows to return VFP_END properly.

Test cases are adjusted accordingly.

Diff is best viewed with -b option

With varnishcache#3809 in place, we can now differentiate properly between the cache size being exactly right and too small.

bsdphk · 2022-05-30T06:04:11Z

We should only attempt to read the next chunked header if we have read-ahead bytes.

Blocking for the next chunked header will break some web-applications which rely on complete concurrent delivery of chunks.

nigoroll · 2022-05-30T12:08:56Z

We should only attempt to read the next chunked header if we have read-ahead bytes.

This is implemented with the last force-push as of 24b9776. Regarding the possible reduction of system calls, I would volunteer to do this next.

I also restructered the commits and noticed another issue on the way: We accepted chunks without the chunk-end (CR)?LF. This is fixed in 4417928

With varnishcache#3809 in place, VFP_END is now signalled opportunistically. Thus, if we see VFP_OK with a full buffer, we still need one extra read to ensure that it does not return VFP_END.

nigoroll · 2022-05-31T17:34:53Z

I have updated this PR with a readahead implementation for chunked encoding.
First things first, for s00003.vtc, this brings down the number of read() calls from v1f_read() from 76 in 55e4a20 to 33 readv() calls in 819a5df (factor 2.3 improvement). This test is not a best case for the patch.
The change is split into three commits:

Prepare state for v1f_chunked_* to hold the buffer
Change v1f_read() to readv() to prepare for reads into the data and readahead buffers
The actual readahead implementation

nigoroll · 2022-06-03T13:27:28Z

Note on the last force-push 3573c33: While working on #3798 together with this PR, I noticed that we can not put the V1F chunked state on any workspace. See commit message of 6d9f54f for details.

With varnishcache#3809 in place, VFP_END is now signalled opportunistically. Thus, if we see VFP_OK with a full buffer, we still need one extra read to ensure that it does not return VFP_END.

dridi · 2022-06-13T22:05:02Z

While this looks overall correct, it also complicates chunked parsing a great deal.

Someone suggested (I think @bsdphk) that maybe it was time to have a receive buffer at the session level where we could always read up to the remaining buffer size and not be worried about cross-request boundaries. I suspect this would also simplify h2 parsing a great deal.

With varnishcache#3809 in place, VFP_END is now signalled opportunistically. Thus, if we see VFP_OK with a full buffer, we still need one extra read to ensure that it does not return VFP_END.

Until now, we read the (CR)?LF at the end of a chunk as part of the next chunk header (see: /* Skip leading whitespace */). For a follow up commit, we are going to want to know if the next chunk header is available for read, so we now consume the chunk end as part of the chunk itself. This also fixes a corner case: We previously accepted chunks with a missing end-of-chunk (see fix of r01729.vtc). Ref: https://datatracker.ietf.org/doc/html/rfc7230#section-4.1

... which we are going to need in a follow up commit. No functional changes, diff best viewed with -b

While working on request body caching improvements, it was noticed that, for chunked encoding, we did not know the request body size before we attempted to read at least one additional byte because the chunked fetch did not return VFP_END with the last chunk, but rather only as a zero-length additional chunk. This commit changes chunked encoding processing to return VFP_END opportunistically if the next chunk header is available. Implementation: Unless there is pipeline readahead data available, the test for available data implies a poll(2) call. Relative to the existing cost of the current implementation, this cost is not considered relevant. To improve efficiency, we should consider a generic readahead for chunked encoding, probably by making the file descriptor non blocking and adding a readahead buffer.

It seems this test now shows no data loss more frequently, which I hope should be fine for the purpose of the test?

This commit contains only the init/fini mechanics for the state/buffer, it is not used yet. The state is allocated on the heap to be prepared for partial caching (see varnishcache#3798), where the V1F layer is left and control returns to VCL: - the worker workspace aws is not an option because it must be freed to its original state before returning to VCL. - the task (req) workspace is not an option because of rollbacks. While I would have preferred to avoid a heap allocation, it will be payed off by the reduction of system calls in follow up commits.

This is to support optional readaheads: The first io vector is for read data which the caller absolutely needs, any other io vectors are for potential readahead. The readahead is not used yet, for now, all v1f_read() calls use the IOV() macro to just wrap the existing single buffer.

We maintain a readahead buffer, which gets filled whenever we parse chunk headers and read chunks. The implementation works at the V1F layer only. Because we currently have no way to return, from this layer, data to the HTC pipeline, we must take great care to never over-read the current request. The existing code treats the CR in the CRLF end sequence for chunked encoding as optional, which, in general, leads to a lower safe readahead than if we required CR. For reading chunks, the readahead case is quite trivial: After each chunk, at least one NL needs to follow, plus the last 0 bytes chunk header (0 + NL), plus at least one NL for the end of trailers, totaling to four bytes of safe readahead. In pratice, usually two bytes will be read ahead, the CRLF ending the chunk. For the final chunk, the safe readahead is too conservative to read all of the usual CRLF 0 CRLF CRLF sequence, but the four bytes readahead is sufficient to trigger reading the final zero chunk header, such that we will return VFP_END properly. For reading chunk headers, the minimal safe readahead assumption is for the shortest possible end chunk, 0 NL NL. We very conservatively update the readahead, such that we will usually process a chunk header with two reads.

With varnishcache#3809 in place, VFP_END is now signalled opportunistically. Thus, if we see VFP_OK with a full buffer, we still need one extra read to ensure that it does not return VFP_END.

nigoroll · 2022-06-25T13:42:02Z

While this looks overall correct, it also complicates chunked parsing a great deal.

I doubt a generic receive buffer, while useful, would change this in any relevant way. We would still need to ensure that the number of bytes required by each parsing step are available. The only thing a generic receive buffer would improve is the readahead, which would not need to be restricted to the current body.

dridi

I found a minor suggestion during a second pass on the patch series.

I'm still thinking we may be taking this problem from the wrong end. The last couple commits in particular drastically increase in complexity despite attempts at containing it.

The chunked header parser in current master is not just incomplete because it may forget to complain about missing [CR]LF delimiters, it's also incapable of parsing a chunked extension (even if it's only to ignore them).

I think we should merge the patch series to the point where VFP_END is sent with the last chunk (and squash the test case stabilization to avoid git-bisect surprises). The read-ahead part should be handled separately.

dridi · 2022-07-12T07:30:08Z

bin/varnishd/http1/cache_http1_vfp.c

+	cll = strtoumax(buf, &q, 16);
+	if (q == NULL || *q != '\0')
+		return (VFP_Error(vc, "chunked header number syntax"));
+	cl = (ssize_t)cll;
+	if (cl < 0 || (uintmax_t)cl != cll)
+		return (VFP_Error(vc, "bogusly large chunk size"));


VNUM_hex()? I'm aware you didn't really touch this part, I simply noticed.

no opinion. Why do we even have vnum_uint () and do not just use strtoumax()?

To optionally work with [b..e) ranges, not just null-terminated strings.

nigoroll · 2022-07-12T08:22:55Z

I'm still thinking we may be taking this problem from the wrong end. The last couple commits in particular drastically increase in complexity despite attempts at containing it.

I honestly do not see the complexity increase. Can you please point to one or two examples?

The chunked header parser in current master is not just incomplete because it may forget to complain about missing [CR]LF delimiters, it's also incapable of parsing a chunked extension (even if it's only to ignore them).

Good point, we should handle extensions, but I believe this topic is orthogonal to this PR and related work.

Being at it, we should also double check if we properly handle trailers. I had made an attempt to get trailer support in ages ago with #2477, but even if we do not add that, we should still at least somehow handle trailers (ignore? error?).

I think we should merge the patch series to the point where VFP_END is sent with the last chunk (and squash the test case stabilization to avoid git-bisect surprises). The read-ahead part should be handled separately.

WFM, any progress is welcome

dridi · 2022-07-12T10:12:35Z

I honestly do not see the complexity increase. Can you please point to one or two examples?

The last 3 commits.

Good point, we should handle extensions, but I believe this topic is orthogonal to this PR and related work.

Agreed.

Being at it, we should also double check if we properly handle trailers. I had made an attempt to get trailer support in ages ago with #2477, but even if we do not add that, we should still at least somehow handle trailers (ignore? error?).

Agreed, we should start with ignoring them. An error is what we already get today when we expect "0[CR]LF[CR]LF" for the last chunk, because that technically embeds the end of trailers.

In a previous discussion we had discussed new vcl_*_end subroutine, and vcl_backend_end would be the place to inspect trailers I suppose. That, in general, poses a storage problem. Also, orthogonal.

An alternative could be to send VFP_END when we see the last chunk (0[CR]LF) and wait for the end of trailers outside of the VFP machinery for a very simple reason: trailers aren't body bytes.

nigoroll · 2022-08-01T14:00:54Z

Bugwash:

can trailer handling be extracted from VFP in any sane way?
Show how trailer support in VCL could look like, ideas:
- explicit body fetch?
- vcl_backend_end ? (but what about request headers ?)

dridi

I was having another look at #4035 and remembered that we have this one open, and now it has a merge conflict to solve.

Anyway, revisiting this I have a few comments:

we can drop 870f16c (since Vip4: add $Restrict in vcc #3915)
870f16c...12e0619 still look good to me (modulus merge conflict)
I'd leave the last 3 commits to be revisited after looking at moving trailers outside of the VFP

dridi · 2024-03-11T09:55:39Z

bin/varnishd/http1/cache_http1_vfp.c

+	if (vfe->priv2 == -1) {
+		vfps = v1f_chunked_hdr(vc, htc, &vfe->priv2);
+		if (vfps != VFP_OK)
+			return (vfps);
 	}


We should introduce a macro for this magic -1.

dridi · 2024-03-11T09:59:39Z

bin/varnishd/http1/cache_http1_vfp.c

+	v1fcp = calloc(1, sz);
+	AN(v1fcp);
+	v1fcp->magic = V1F_CHUNKED_PRIV_MAGIC;


We could now use ALLOC_FLEX_OBJ() here.

nigoroll · 2024-05-29T11:20:48Z

Following up here on the topic of trailer support, motivated by #4035 (comment)

As a personal preface, I would like to add that this triggers some frustration on my end. take this iteratively and not involve VCL support on day 1 was exactly what I tried to do in #2477 and various other tickets. It irritates me that when I mention the feedback (pushback, if you will) which I received at the time from the same group of people now working on a new attempt, things seem to have changed and "not having a clear VCL concept" suddenly seems to be OK.

So now that I got this off my chest, let's leave the past behind - I also want to have trailer support and I will be happy to get back to these old tickets and make progress. Also I do want to collaborate.

How to we proceed here? Do we have any new suggestions on the table?

dridi · 2024-06-04T11:37:41Z

I offer you my apologies if I contributed to your frustration on this specific topic. Until you brought it up, I didn't remember your former attempt at dealing with trailers.

Here is where we stand right now, based on #3809 (review):

we cherry-picked the commits I mentioned as a starting point
we moved the trailer fetch outside of the VFP stack (implemented in V1L, I think)
- for beresp this requires a new VDI method (implemented in VBE)
- for req this requires pipelining adjustments

At this point, first milestone, we preserve HTTP/1 framing in the presence of trailers, but we effectively drop them. I believe @walid-git got this working already (he's doing most of the work, when I say "we" I mostly refer to him).

What we are planning next:

h2 trailers for req
storing trailers (new object attribute)
forwarding trailers as-is back and forth for pass transactions
- guarded by an experimental::pass_trailers flag

This second milestone would allow us to see how well Varnish integrates with applications relying on trailers, with caveats such as the risk of breaking check sums when filters are involved, hence the initial experimental status. I believe this also becomes a good basis to discuss how to expose trailers to VCL.

I will try to revisit #2477 and see what we may reconsider with our current plan.

edit: I submitted prematurely because of ctrl+return keyboard accident.

nigoroll mentioned this pull request May 28, 2022

std.cache_req_body(BYTES size, BOOL partial = 0) #3798

Draft

nigoroll added a commit to nigoroll/varnish-cache that referenced this pull request May 29, 2022

Fix cached request body size limitation with chunked TE

49bae51

With varnishcache#3809 in place, we can now differentiate properly between the cache size being exactly right and too small.

nigoroll force-pushed the v1f_chunked_vfp_end branch from ec5c816 to cda5b46 Compare May 29, 2022 12:30

nigoroll added a commit to nigoroll/varnish-cache that referenced this pull request May 29, 2022

Fix cached request body size limitation with chunked TE

bbb42cd

With varnishcache#3809 in place, we can now differentiate properly between the cache size being exactly right and too small.

nigoroll force-pushed the v1f_chunked_vfp_end branch from cda5b46 to a4f0921 Compare May 30, 2022 12:05

nigoroll mentioned this pull request May 30, 2022

V1F: Read end-of-chunk as part of the chunk #3811

Open

nigoroll force-pushed the v1f_chunked_vfp_end branch 2 times, most recently from 9226b17 to 7c1160e Compare May 31, 2022 10:09

nigoroll changed the title ~~Send VFP_END with last data for v1f chunked~~ Send VFP_END with last data for v1f chunked & read ahead May 31, 2022

nigoroll force-pushed the v1f_chunked_vfp_end branch from 819a5df to 3573c33 Compare June 3, 2022 13:23

nigoroll force-pushed the v1f_chunked_vfp_end branch from 3573c33 to c9ce836 Compare June 24, 2022 08:48

nigoroll force-pushed the v1f_chunked_vfp_end branch from c9ce836 to 38c3499 Compare June 24, 2022 14:43

nigoroll added 8 commits June 24, 2022 16:56

Avoid panic in VRT_CacheReqBody if called from vcl_init{}

870f16c

V1F: pull chunk header parsing into an own function

1a3f9fc

... which we are going to need in a follow up commit. No functional changes, diff best viewed with -b

Stabilize partial write -sdeprecated_persistent test

12e0619

It seems this test now shows no data loss more frequently, which I hope should be fine for the purpose of the test?

nigoroll force-pushed the v1f_chunked_vfp_end branch from 38c3499 to fdf390c Compare June 24, 2022 14:57

dridi reviewed Jul 12, 2022

View reviewed changes

dridi reviewed Mar 11, 2024

View reviewed changes

dridi mentioned this pull request May 28, 2024

Add req.filters and bereq.filters #4035

Merged

dridi mentioned this pull request Jun 18, 2024

http: Skip req and beresp trailers #4125

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send VFP_END with last data for v1f chunked & read ahead #3809

Send VFP_END with last data for v1f chunked & read ahead #3809

nigoroll commented May 28, 2022

bsdphk commented May 30, 2022

nigoroll commented May 30, 2022

nigoroll commented May 31, 2022

nigoroll commented Jun 3, 2022

dridi commented Jun 13, 2022

nigoroll commented Jun 25, 2022

dridi left a comment

dridi Jul 12, 2022

nigoroll Jul 12, 2022 •

edited

Loading

dridi Jul 12, 2022

nigoroll commented Jul 12, 2022

dridi commented Jul 12, 2022

nigoroll commented Aug 1, 2022 •

edited

Loading

dridi left a comment

dridi Mar 11, 2024

dridi Mar 11, 2024

nigoroll commented May 29, 2024 •

edited

Loading

dridi commented Jun 4, 2024 •

edited

Loading

Send VFP_END with last data for v1f chunked & read ahead #3809

Are you sure you want to change the base?

Send VFP_END with last data for v1f chunked & read ahead #3809

Conversation

nigoroll commented May 28, 2022

bsdphk commented May 30, 2022

nigoroll commented May 30, 2022

nigoroll commented May 31, 2022

nigoroll commented Jun 3, 2022

dridi commented Jun 13, 2022

nigoroll commented Jun 25, 2022

dridi left a comment

Choose a reason for hiding this comment

dridi Jul 12, 2022

Choose a reason for hiding this comment

nigoroll Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

dridi Jul 12, 2022

Choose a reason for hiding this comment

nigoroll commented Jul 12, 2022

dridi commented Jul 12, 2022

nigoroll commented Aug 1, 2022 • edited Loading

dridi left a comment

Choose a reason for hiding this comment

dridi Mar 11, 2024

Choose a reason for hiding this comment

dridi Mar 11, 2024

Choose a reason for hiding this comment

nigoroll commented May 29, 2024 • edited Loading

dridi commented Jun 4, 2024 • edited Loading

nigoroll Jul 12, 2022 •

edited

Loading

nigoroll commented Aug 1, 2022 •

edited

Loading

nigoroll commented May 29, 2024 •

edited

Loading

dridi commented Jun 4, 2024 •

edited

Loading