Return a consistent boc state from ObjWaitExtend()

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.

bin/varnishd/cache/cache_obj.c

@@ -265,8 +265,10 @@ ObjExtend(struct worker *wrk, struct objcore *oc, ssize_t l, int final)
  */
 
 uint64_t
-ObjWaitExtend(const struct worker *wrk, const struct objcore *oc, uint64_t l)
+ObjWaitExtend(const struct worker *wrk, const struct objcore *oc, uint64_t l,
+    enum boc_state_e *statep)
 {
+	enum boc_state_e state;
 	uint64_t rv;
 
 	CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
@@ -282,15 +284,16 @@ ObjWaitExtend(const struct worker *wrk, const struct objcore *oc, uint64_t l)
 			oc->boc->delivered_so_far = l;
 			PTOK(pthread_cond_signal(&oc->boc->cond));
 		}
-		if (rv > l || oc->boc->state >= BOS_FINISHED)
+		state = oc->boc->state;
+		if (rv > l || state >= BOS_FINISHED)
 			break;
 		(void)Lck_CondWait(&oc->boc->cond, &oc->boc->mtx);
 	}
-	rv = oc->boc->fetched_so_far;
 	Lck_Unlock(&oc->boc->mtx);
+	if (statep != NULL)
+		*statep = state;
 	return (rv);
 }
-
 /*====================================================================
  */
 

bin/varnishd/cache/cache_varnishd.h

@@ -335,7 +335,7 @@ void ObjDestroy(const struct worker *, struct objcore **);
 int ObjGetSpace(struct worker *, struct objcore *, ssize_t *sz, uint8_t **ptr);
 void ObjExtend(struct worker *, struct objcore *, ssize_t l, int final);
 uint64_t ObjWaitExtend(const struct worker *, const struct objcore *,
-    uint64_t l);
+    uint64_t l, enum boc_state_e *statep);
 void ObjSetState(struct worker *, const struct objcore *,
     enum boc_state_e next);
 void ObjWaitState(const struct objcore *, enum boc_state_e want);

bin/varnishd/storage/storage_simple.c

@@ -362,23 +362,16 @@ sml_iterator(struct worker *wrk, struct objcore *oc,
 	}
 	while (1) {
 		ol = len;
-		nl = ObjWaitExtend(wrk, oc, ol);
-		if (boc->state == BOS_FAILED) {
+		nl = ObjWaitExtend(wrk, oc, ol, &state);
+		if (state == BOS_FAILED) {
 			ret = -1;
 			break;
 		}
 		if (nl == ol) {
-			/*
-			 * note: the unguarded boc->state read could be
-			 * outdated, in which case we call ObjWaitExtend() again
-			 * for error handling but otherwise cause no harm. When
-			 * using this code as an example, DO NOT rely on
-			 * boc->state to be consistent
-			 */
-			if (boc->state == BOS_FINISHED)
-				break;
-			continue;
+			assert(state == BOS_FINISHED);
+			break;
 		}
+		assert(nl > ol);
 		Lck_Lock(&boc->mtx);
 		AZ(VTAILQ_EMPTY(&obj->list));
 		if (checkpoint == NULL) {
@@ -417,7 +410,6 @@ sml_iterator(struct worker *wrk, struct objcore *oc,
 		st = VTAILQ_PREV(st, storagehead, list);
 		if (st != NULL && st->len == 0)
 			st = NULL;
-		state = boc->state;
 		Lck_Unlock(&boc->mtx);
 		assert(l > 0 || state == BOS_FINISHED);
 		u = 0;

doc/changes.rst

@@ -41,6 +41,11 @@ Varnish Cache NEXT (2024-09-15)
 .. PLEASE keep this roughly in commit order as shown by git-log / tig
    (new to old)
 
+* The ObjWaitExtend() Object API function gained a ``statep`` argument
+  to optionally return the busy object state consistent with the
+  current extension. A ``NULL`` value may be passed if the caller does
+  not require it.
+
 * for backends using the ``.via`` attribute to connect through a
   *proxy*, the ``connect_timeout``, ``first_byte_timeout`` and
   ``between_bytes_timeout`` attributes are now inherited from *proxy*

include/vrt.h

@@ -48,7 +48,7 @@
 
 #define VRT_MAJOR_VERSION	19U
 
-#define VRT_MINOR_VERSION	0U
+#define VRT_MINOR_VERSION	1U
 
 /***********************************************************************
  * Major and minor VRT API versions.
@@ -58,6 +58,8 @@
  * binary/load-time compatible, increment MAJOR version
  *
  * NEXT (2024-09-15)
+ * 19.1 (2024-05-27)
+ *	[cache_varnishd.h] ObjWaitExtend() gained statep argument
  * 19.0 (2024-03-18)
  *	[cache.h] (struct req).filter_list renamed to vdp_filter_list
  *	order of vcl/vmod and director COLD events reversed to directors first