Add ObjWaitExtend2() to provide a consistent boc state #4109
Conversation
nigoroll
force-pushed
the
ObjWaitExtend2
branch
2 times, most recently
from
705b1e5 to
7205897
Compare
include/vrt.h
Outdated
| * 19.1 (xxx) | ||
| * ObjWaitExtend() gained statep argument |
There was a problem hiding this comment.
Not a VRT change, shouldn't trigger a VRT bump.
There was a problem hiding this comment.
We have a long tradition of recording many more changes in vrt.h than changes to functions prefixed VRT_, and we have not even defined what VRT really is (#3355).
Using the VRT version to mark changes like this one GREATLY simplifies use in vmods/vexts, in particular in light of the de-autotoolification at zero + epsilon cost. So what is the gain of NOT using vrt logs and versions?
There was a problem hiding this comment.
We've already been there, I find it confusing to make a VRT bump for something other than a VRT change. And that includes the systematic "better safe than sorry" systematic major (!) bump for biannual releases.
This particular function is in cache_varnishd.h, which should settle the VRT-or-not question in this case.
Note that I approved the pull request and didn't even try to "request changes". I'm merely mentioning that this should say "NEXT" instead of "19.1" but I'm not vetoing it.
There was a problem hiding this comment.
Yes, I did notice the approval, thank you! Also, I gave a wrong references, which I fixed above. Apologies for that.
I agree with you on the "just in case bump", but the Obj* API is really misplaced in cache_varnishd.h since VEXTs are a thing. As we^WI did not even make progress with #3355 it's obviously not important enough.
There was a problem hiding this comment.
I agree that VEXTs are a different beast of their own.
|
bugwash: phk agrees |
Clients to the Object API need to know not only the current extension (new length) of streaming objects, but also the streaming state - in particular BOS_FINISHED and BOS_FAILED. The latter for obvious reasons, and the former to call the delivery function with OBJ_ITER_END, which then likely results in VDP_END sent down the delivery pipeline. Background: It is important for efficient delivery to not receive an additional VDP_END with a null buffer, but rather combined with the last chunk of data, so, consequently, it is important to reliably send OBJ_INTER_END also with the last chunk of data. Consequent to all of this, ObjWaitExtend() callers need to know when BOS_FINISHED has been reached for some extension. The current API, however, does not provide a consistent view of the streaming state, which is only available from within the critical region of ObjWaitExtend(). Thus, we add the streaming state as an optional return value. With this commit, we also remove a superfluous line to set rv again: Because boc->fetched_so_far must only be updated while holding the boc mutex, reading the value again provides no benefit.
For context see SLASH/fellow issue #44.
Clients to the Object API need to know not only the current extension (new length) of streaming objects, but also the streaming state - in particular
BOS_FINISHEDandBOS_FAILED. The latter for obvious reasons, and the former to call the delivery function withOBJ_ITER_END, which then likely results inVDP_ENDsent down the delivery pipeline.Background:
It is important for efficient delivery to not receive an additional
VDP_ENDwith a null buffer, but rather combined with the last chunk of data, so, consequently, it is important to reliably sendOBJ_INTER_ENDalso with the last chunk of data.Consequent to all of this,
ObjWaitExtend()callers need to know whenBOS_FINISHEDhas been reached for some extension.The current API, however, does not provide a consistent view of the streaming state, which is only available from within the critical region of
ObjWaitExtend().Thus, we add the streaming state as an optional return value.
With this commit, we also remove a superfluous line to set
rvagain: Becauseboc->fetched_so_farmust only be updated while holding thebocmutex, reading the value again provides no benefit.