req_fsm: Always run vcl_synth upon client req reset #4412
Conversation
This way it becomes possible to log extra information for log consumers, call into VMODs, or whatever else one might want to do for a synthetic 408 response.
| * worker thread. We return to STP_LOOKUP when the busy | ||
| * object has been unbusied, and still have the objhead | ||
| * around to restart the lookup with. | ||
| * worker thread. We return to R_STP_LOOKUP, with the |
There was a problem hiding this comment.
the commit message sounds like there would be more to it, but this is really just a comment fix?
There was a problem hiding this comment.
Yes, this describes the old waiting list system with the retired hash_objhead field. I noticed this while surveying the req_reset behavior because we have more waiting list features on the Enterprise side (primarily #3835).
There was no reason to omit it from this patch series.
There was a problem hiding this comment.
I was really only asking because the commit message " req_fsm: Clear stale reference to req->hash_objhead" sounds like you had other intentions than only polishing a comment.
| * One is that sub vcl_recv is always followed by a call to sub | ||
| * vcl_hash to always compute a cache key. The other one is that | ||
| * we always want to run sub vcl_synth for return(fail) even if | ||
| * we won't deliver the response, and maybe sub vcl_synth needs |
There was a problem hiding this comment.
I do not understand this. The only place where req_reset is set, VCL_RET_FAIL handling is signaled. Why does this pivot around req_reset and not VCL_RET_FAIL as already seems to be the case?
Also, is vcl_synth{} still the right place for this? Do we need some place to handle VCL_RET_FAIL, where it is clear that we do not emit headers?
I do not want to say that this is wrong, but could you please at least explain more?
There was a problem hiding this comment.
The req_poll() function is a short-circuit to circumvent VCL execution once a request is "reset". For now, that is limited to aborted h2 connections or h2 streams being reset (and again, #3835 and more on the enterprise side).
The goal is to ensure that this short circuit either emulates a return (fail) that normally leads to vcl_synth (via R_STP_VCLFAIL) but does not prevent vcl_synth from being executed.
There was a problem hiding this comment.
I do not understand how your response would answer my questions.
-
why does this (need to) look at
req_resetand not just atVCL_RET_FAIL? -
Would it not be cleaner to have a vcl sub to handle
VCL_RET_FAILrather than invokingvcl_synth {}, but without the intended effect? The existing code path invcl_synth {}is to handle areturn(fail)fromvcl_synth {}itself, but now enteringvcl_synth {}for fail paths and not executing the expected delivery seems plain wrong to me.
Side note: We should base our design decisions on what we have in our code base. What is in your private fork should not guide us.
|
This feels too adhoc and "forced" |
Forced, for sure. Since
Is this ad-hoc core code handling or acting like a
I think we can have both. The point that was raised on our end was that an emulated failure should be emulated all the way to |
if we added, say, a |
3 participants
This way it becomes possible to log extra information for log consumers, call into VMODs, or whatever else one might want to do for a synthetic 408 response.