varnishcache / varnish-cache

Varnish Cache source code repository
https://www.varnish-cache.org
Other
3.71k stars 378 forks source link

RFC how to handle failures at esi_level > 0 #3264

Open nigoroll opened 4 years ago

nigoroll commented 4 years ago

I am going to merge #3242 now because I think it qualifies for "trivial fix", I had opened it during the freeze and also it has one approval.

There is an important aspect to retain from that ticket: Can we / do we want to improve error reporting for the case that an ESI subrequest fails?

Right now, we just drop that segment, such that parts of the ESI-generated page become missing, which I think is the worst of all options.

Suggestions from #3242 are:

bsdphk commented 4 years ago

We hashed this at bugwash today, and various things came up:

There are two distinct cases:

A) The subrequest have pushed bytes into VDP but fails before it's done.

This only happens if the subrequest is streaming and the backend fails.

This can only be be avoided by not streaming the subrequest.

Not sure what our error handling is today, or indeed what it should be. Probably ought to abort the entire delivery, as there may be strange partial content in the delivery.

B) The subrequest has not yet pushed any bytes into VDP.

In this case the parent could have exeception handling.

Currently we just ignore and move on.

We could implement ESI's alt= attribute.

Using alt= a red flag could be delivered by vcl_synth{}

See also: ESI's onerror=

bsdphk commented 3 years ago

Next step is when somebody writes some code.

nigoroll commented 1 year ago

I'd like to briefly talk about this ticket again during bugwash: are we happy with the onerror= change from #3767 or do we want alt= also, or even something else which we did not mention so far?

nigoroll commented 1 month ago

homework submission: With #4053 fixed by cffda50c8bb65206b1709a16ae6138264907e0a8, I think all that is missing at this point is VCL control over the flag. With that in place, I think, one could implement an onerror fallback from vcl also if no onerror attribute is present. Do I overlook anything or would we then have all cases covered?

nigoroll commented 3 weeks ago

bugwash: