Add req.filters and bereq.filters #4035
Conversation
nigoroll
force-pushed
the
req_filters_4
branch
from
6b4bcdd
to
dd2d224
Compare
There was a problem hiding this comment.
I feel that instead of breaking so much of the API to enable it for *req.body we could instead enhance the vdp_ctx and add a bunch of fields (bo, hd and cl) and let processors check or use what they need from the context (similarly to vrt_ctx).
For example, the ESI processor could assert(vdc->hd == req->resp) after grabbing reqand we wouldn't need to change much more.
That should also mean changing this prototype:
void VDP_Init(struct vdp_ctx *, struct *req, struct *bo);Considering how it currently operates strictly on req fields it would probably be an improvement anyway. And that would open the pipe case, that I didn't see addressed in this patch series (but this was not a thorough review so don't be mad if pipe is covered).
Now regarding the objcore, we could imagine something like this for VDPs that need it:
struct objcore * VDP_Objcore(struct vdp_ctx *);It would check whether this is the first member of the stack and grab the right one depending on the calling context.
Overall I'm in favor but I get the feeling that this change unpacks too much of the API instead of keeping as much as relevant contained.
| client c1 { | ||
| # Start with enough workspace to receive a good result | ||
| txreq -hdr "WS: 320" | ||
| # Start with enough workspace to receive some good results | ||
| client c1 -repeat 5 { | ||
| txreq -hdr "Connection: close" | ||
| rxresp | ||
| expect resp.status == 200 | ||
| } -run |
There was a problem hiding this comment.
Maybe submit that kind of change independently?
| @@ -1,6 +1,9 @@ | |||
| varnishtest "sweep through tight backend workspace conditions" | |||
|
|
|||
| # XXX almost the same as r01834.vtc - retire? | |||
There was a problem hiding this comment.
Probably not if r1834 didn´t catch #2645 at the time.
There was a problem hiding this comment.
r1834 was not originally using the "sweep" idea. Now I have changed it to also use that, so now they are almost the same.
| VDP_Push(VRT_CTX, struct vdp_ctx *vdc, struct ws *ws, const struct vdp *vdp, | ||
| void *priv) | ||
| VDP_Push(VRT_CTX, struct vdp_ctx *vdc, struct ws *ws, | ||
| const struct vdp *vdp, void *priv, | ||
| struct objcore *oc, struct req *req, | ||
| struct http *hd, intmax_t *cl) |
There was a problem hiding this comment.
Nit pick, the priv argument tends to be first in callbacks and last otherwise (though not always).
There was a problem hiding this comment.
I did ponder this question for a bit: The priv argument is the priv member from struct vdp_entry, so it belongs to the vdp. Thus I kept it next to it.
These questions are, in my mind, answered in the commit message of 0274ebc: We already have (had, from the perspective of the patch) a I think the API should make it clear that VDPs should not mess with a request, a busy object, headers or the content length after init time, thus I think that we should change the init function signature and specifically not add Also please note the following from the commit message regarding the motivation for this change:
I did actually try this for VDPs and ended up with repetitive code - you basically have to repeat the additions to VCL_StackVDP() in each VDP init function. The changed API makes it easier (and, in my mind, clearer) how to write a VDP which works everywhere: You get the headers and the content length, make modifications to both as needed and be done. See for example the vmod_re change, which mostly consists of boilerplate changes to For VDPs which need objcore access or only work on the client side, I believe, the API change also clarifies the situation: The respective init function arguments are present only if the respective access is allowed.
Please look at the commit again, it also does not change much more: Besides the
I think we should work on one question at a time, and if pipe mode requires another API change, then so be it. But does it? |
|
There is a leak reported by ASAN: |
nigoroll
force-pushed
the
req_filters_4
branch
from
109ad13
to
ba78308
Compare
This is not going through a VDP, but it eventually will. Refs varnishcache#4035
nigoroll
force-pushed
the
req_filters_4
branch
from
ba78308
to
e851210
Compare
|
I have rebased, squashed the squashable commits and force-pushed . |
This commit is to prepare for use of the VDP API also for the backend side to filter bereq.body through bereq.filters: The req member of struct vdp_ctx is removed, because it should only be used for initialization and, consequently, is already nulled in VDP_DeliverObj(). vdp_init_f() and its caller VDP_Push() gain arguments: - req is only present if the VDP is used on the client side. - hd is a pointer to the outgoing headers corresponding to the body being worked on: (struct req).resp on the client side and (struct busyobj).bereq on the backend side (tbd). - cl is a pointer to the content-length. -1 indicates 'unknown'. VDPs should aim for compatibility with both the client and backend use case by using only hd and cl instead of req. If they depend on req, they should return an error if req is NULL. Note that none of the new arguments would be necessary, they could all (including the existing oc argument) be derived from the VRT_CTX. The reasons for having these arguments are code clarity and avoidance of code duplication, e.g. for repeatedly figuring out the correct header and length pointer on the client vs. backend side.
This change introduces additional failure points to V1F_SendReq() when there is insufficient workspace to push the V1L VDP. We adjust r01834.vtc to cover the respective 503 errors and simplify it by example of r02645.vtc. The two test cases now resemble each other a lot, but I would think, at least for now, their purpose is so important that the overhead does not matter.
Summary
These changes add support for request body filters on the client and backend side. The motivation for this feature will hopefully be obvious: To be able to transform or process "uploads" in varnish-cache without the need for a backend application.
VDPs and VFPs are used here "in reverse" to the existing use on response bodies: the client side fetches the request body, so it uses a VFPs, while the backend side delivers the request body to be backend side, so it uses VDPs.
Please read the individual commit messages. This description is intended so serve as an overview.
Preparation work
This PR is based on some minor changes which were pushed directly to trunk because they do not represent API changes, preparing VDP for use on the backend side, where we do not have a
struct req:vdp_ctxesstruct reqfrom VDPs to where absolutely necessary (client side only VDPs).(struct req).filter_listfor consistencyOBJ_ITER_ENDflag to the request bodyobjiterate_fcallOverview of commits
The first commit generalizes the VDP API to make it suitable for use on the backend side. The main change here is to pass, for generic VDPs (those which are suitable for backend side use), the "outgoing" headers and a pointer to a length field, rather than using
struct req. As is, this change serves clarity and code compaction only, all information is already present in theVRT_CTXargument, but theclargument becomes important later on.The next two commits add VDPs for
bereq.bodywithout introducing custom VDPs yet. That is, only the http1 VDP is pushed, keeping functionality otherwise. As a consequence of this change, fetches can now also fail for out-of-workspace for VDP.Prepare VCL_StackVDP for backend use changes the implementation to work either on a
struct reqorstruct busyobj, but not both.Once this groundwork is laid, adding the actual fields and VRT interface for
req.filtersandbereq.filtersis rather trivial. We stack the filters like elsewhere and add a test case.To be done after merge
changelog