Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC how to handle failures at esi_level > 0 #3264

Open
nigoroll opened this issue Apr 1, 2020 · 5 comments
Open

RFC how to handle failures at esi_level > 0 #3264

nigoroll opened this issue Apr 1, 2020 · 5 comments

Comments

@nigoroll
Copy link
Member

nigoroll commented Apr 1, 2020

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:

  • Would it make sense to - literally - insert a (canned) red flag into the markup? If it was markup, users of the web page could provide feedback, if it wasn't, the flag would most likely render whatever is being rendered invalid.
    That canned red flag would have a plain and a gzip variant, depending on the compression context, and would update the parent's CRC etc.
  • Can we interrupt the delivery? Sudden close of the session or RST of the stream?
  • Can we push the VDPs earlier and not find out about the problem in the middle of the delivery?
@bsdphk
Copy link
Contributor

bsdphk commented Aug 3, 2020

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
Copy link
Contributor

bsdphk commented Aug 25, 2021

Next step is when somebody writes some code.

@bsdphk bsdphk closed this as completed Aug 25, 2021
@nigoroll
Copy link
Member Author

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
Copy link
Member Author

homework submission: With #4053 fixed by cffda50, 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
Copy link
Member Author

nigoroll commented Nov 4, 2024

bugwash:

  • @walid-git volunteered to add vcl visibility of and control over the onerror setting in effect for esi subrequests. thank you
  • it seems this needs better documentation, for example that onerror=continue might deliver garbled responses when streaming non-esi fragments, while disabling streaming and having an appropriate vcl_synth {} will avoid that
  • the buildin.vcl should, by default, do that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants