[jira] [Updated] (TS-3979) Remove allow empty doc cache setting.

["Sudheer Vinukonda (JIRA)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/TS-3979?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Sudheer Vinukonda updated TS-3979:
----------------------------------
    Affects Version/s: 6.0.0
        Fix Version/s: 7.0.0

> Remove allow empty doc cache setting.
> -------------------------------------
>
>                 Key: TS-3979
>                 URL: https://issues.apache.org/jira/browse/TS-3979
>             Project: Traffic Server
>          Issue Type: Bug
>          Components: Cache
>    Affects Versions: 6.0.0
>            Reporter: Sudheer Vinukonda
>             Fix For: 7.0.0
>
>
> Refer TS-3978
> Following a discussion on the IRC, opening this jira to deprecate the Allow empty doc cache setting. The consensus is to cache anything that's cacheable and valid (regardless, of if it's empty or non-empty).
> {code}
> Amaryllis
> 7:50
> sudheerv: do any other reverse proxies refuse to cache empty documents without content-length?  it's not something i've noticed before (in varnish, for example)
> Amaryllis
> 7:50
> the connection could still be broken at any point in the response and result in broken content being cached
> 7:51
> but as a compromise, what if there was another option to only cache empty responses for 3xx, which is probably 80% of use cases?
> 7:52
> (maybe, in addition, it should accept transfer-encoding: chunked, since that can indicate end-of-body)
> zwoop_
> 7:54
> Amaryllis  we should talk to amc (he knows everything, we don't need Google here) about this
> 7:54
> zwoop_ is now known as woop
> 7:54
> woop is now known as zwoop
> 7:54
> zwoop left the room (quit: Changing host).
> 7:54
> zwoop [~zwoop@apache/committer/zwoop] entered the room.
> 7:54
> mode (+o zwoop) by ChanServ
> zwoop
> 7:55
> Amaryllis amc Maybe we could have a plugin API, that basically forces to set f.allow_empty_doc ?
> amc
> 7:55
> I think you can change the CacheVC structure without problem.
> Amaryllis
> 7:55
> i think some sort of fix for this should be in core, at the moment TS won't cache any of our redirects
> amc
> 7:56
> There's not a global config to allow that?
> zwoop
> 7:56
> Amaryllis  does it send a Cache-Control header with the redirects ?
> Amaryllis
> 7:56
> amc, zwoop: the origin sends transfer-encoding: chunked instead of content-length.
> 7:56
> so even with cache-control it won't be cached, that's what this PR is to change
> 7:56
> TS-3978
> ASFBot
> 7:56
> TS-3978: Allow empty document caching to follow normal logic - https://issues.apache.org/jira/browse/TS-3978
> zwoop
> 7:56
> ah
> 7:57
> yeah, that's expected
> 7:57
> and is why we added that option to allow empty docs to be cached
> Amaryllis
> 7:57
> this is with that option enabled
> zwoop
> 7:57
> huh
> Amaryllis
> 7:57
> it still won't cache any document without content-length
> zwoop
> 7:57
> yeah, need CL: too
> 7:57
> that's a requirement
> Amaryllis
> 7:57
> so i'll update the PR to also recognise chunked responses as being cacheable even if empty
> zwoop
> 7:57
> otherwise, it can't distinguish the cases you were worried about (a broken connection) vs a truly empty doc
> Amaryllis
> 7:58
> zwoop: right, but the PR makes it optional, only if allow_empty_doc=2
> zwoop
> 7:58
> heh, your redirect is Chunked ??
> Amaryllis
> 7:58
> yes, an empty chunked response 
> zwoop
> 7:58
> is that even correct?
> Amaryllis
> 7:58
> it's perfectly valid, if somewhat unusual
> zwoop
> 7:58
> doesn't the chunking require the final "0" ?
> 7:58
> which means, the doc isn't empty
> Amaryllis
> 7:58
> i think ATS is looking at 'empty' after de-chucking, isn't it?
> 7:59
> because it's definitely not cacheing these responses
> zwoop
> 7:59
> not sure
> 7:59
> gancho_ left the room (quit: Ping timeout: 272 seconds).
> amc
> 7:59
> I think Amaryllis is correct.
> zwoop
> 7:59
> Amaryllis  but, I definitely remember that the CL: header was a requirement that can not be ignored (safely), so not sure I think having an allow_empty_doc=2 is ok
> 8:00
> unless allow_empty_doc=2 also means allow Chunked content to be empty
> amc
> 8:00
> I need to check to see what ATS actually caches - the chunked data or the payload.
> zwoop
> 8:00
> but, you have to have some indicator telling us that the doc really is empty before we can cache it
> 8:00
> amc is caches the unchunked data
> 8:00
> it dechunks it, and caches that
> 8:01
> is dechunk even a proper verb? 
> amc
> 8:01
> Ah, but it caches the encoding header.
> Amaryllis
> 8:01
> zwoop: https://dpaste.de/6kUT
> zwoop
> 8:01
> amc Hmmm, really ? That doesn't sound right
> amc
> 8:01
> "zwoop drank too much at the summit and dechunked himself in the hallway". Yep, a proper verb.
> Amaryllis
> 8:01
> that's the actual origin response causing problems for us
> zwoop
> 8:01
> amc I'd expect it to change it from chunked to a Content-Length:
> amc
> 8:02
> I was wondering about that.
> zwoop
> 8:02
> amc lets dechunk big time at the Bar camp
> amc
> 8:02
> What happens when the content is served? Is the encoding preserved and obeyed?
> zwoop
> 8:03
> for the first client (the cache miss) I think it seems the chunked response
> Amaryllis
> 8:03
> yes, TS returns a correct thunked response
> 8:03
> and never caches it, so it's always the same
> zwoop
> 8:03
> but subsequent requests (when served out of cache) should have a CL:
> amc
> 8:03
> I was thinking of what happens for chunked responses from origin that are non-zero lenght.
> zwoop
> 8:03
> Amaryllis  can you not convince your origin that they are crazy, and change it to not send a Chunked empty response, and instead send CL: 0 ?
> sudheerv
> 8:04
> catching up on the messages…but, my concern is the same as zwoop's
> Amaryllis
> 8:04
> zwoop: no, it never adds a CL: https://dpaste.de/Goex
> zwoop
> 8:04
> Amaryllis  because youa re not caching it
> Amaryllis
> 8:04
> yes, exactly, because it's not cacheable 
> zwoop
> 8:04
> Amaryllis  amc is asking the general question, to which my answer is
> Amaryllis
> 8:04
> ah
> zwoop
> 8:04
> amc yes, I'd expect it to send the chunked response to the first client, and subsequent responses (out of cache) with a CL: 0
> Amaryllis
> 8:04
> perhaps i can make uwsgi do some sort of output buffering and put a CL header in
> zwoop
> 8:05
> amc that's at least how it's worked for the last ~10 years, we better not ahve changed it
> Amaryllis
> 8:05
> zwoop: how can that ever happen?  a chunked empty response will never be cached
> zwoop
> 8:05
> not talking empty responses
> 8:05
> general case
> amc
> 8:05
> I have many opinions on this. I do think it should be cacheable as an empty doc if it's chunked with a zero length payload.
> Amaryllis
> 8:05
> okay, but it's not CL: 0 then
> zwoop
> 8:05
> your case is deranged
> 8:05
> Amaryllis  right, nothing to do with your case
> Amaryllis
> 8:05
> amc: shall i submit a separate PR to fix that?
> amc
> 8:06
> Yes.
> zwoop
> 8:06
> amc yeah, that seems reasonable. But, If it was me, I'd still beat the Origin people over the head, cause they are crazy
> sudheerv
> 8:06
> amc: yes, but, you have to ensure that you received the final chunk
> 8:06
> if it already is the case, by the time you are checking for allow empty doc caching, then that's fine
> amc
> 8:06
> Right. That is happening in the clips Amaryllis showed us.
> Amaryllis
> 8:06
> zwoop: i think it's because the origin sends the headers before it's generated the response body.  so it doesn't know it's going to be empty
> sudheerv
> 8:06
> but, i dont know if that's the case already
> zwoop
> 8:07
> Amaryllis  it would know it's a redirect no?
> 8:07
> Amaryllis and why would a redirect have a body, ever ?
> Amaryllis
> 8:07
> yes, but redirects can still have bodies
> sudheerv
> 8:07
> you may get empy responses without content-length when we support outbound spdy/h2 as well
> zwoop
> 8:07
> true
> Amaryllis
> 8:07
> most web servers include bodies in 3xx
> 8:07
> i did have the origin fix it in one specific case but we need something more general
> zwoop
> 8:07
> yeah, I forgot about that, you're right
> 8:07
> niq left the room (quit: Ping timeout: 250 seconds).
> amc
> 8:08
> If it's chunked and ATS recieves the final payload (0 length frame) it can reasonably presume it got valid (non-truncated) content.
> sudheerv
> 8:08
> Amaryllis: I think the patch should ensure it can cover non-broken empty body cases for chunked-encoding/spdy/h2
> Amaryllis
> 8:08
> sudheerv: does this even apply to spdy/h2?
> Amaryllis
> 8:09
> actually, there is no H2 to origins at all...
> sudheerv
> 8:09
> not right now - we don't support outbound spdy/h2 yet
> Amaryllis
> 8:09
> right
> sudheerv
> 8:09
> but, when we do, it will be a similar problem there
> Amaryllis
> 8:09
> yes
> sudheerv
> 8:09
> there's no chunked encoding or content-len with spdy/h2
> Amaryllis
> 8:09
> but i can't really test code that's yet to be written 
> sudheerv
> 8:09
> so, yet another case of empty body 
> 8:09
> no, my point is - the code we write now must not break when we have spdy/h2
> 8:10
> so, instead of "assuming" that the body is valid, it might make sense
> 8:10
> to somehow indicate that it is
> Amaryllis
> 8:10
> well, i think it will break anyway, in the sense that empty H2 responses won't be cached... since they have no CL?
> sudheerv
> 8:10
> well, but you are fixing that 
> Amaryllis
> 8:10
> but H2 doesn't do TE either, does it?  i thought it replaces that entire thing
> sudheerv
> 8:10
> if it's consistently broken (or not supported), that's alright
> 8:10
> yes, it doesn't
> 8:10
> that's my point
> Amaryllis
> 8:11
> all i'm adding (for now) is to make it accept CL _or_ TE
> sudheerv
> 8:11
> so, we need to have a generic way of notifying cache layer
> 8:11
> that the response is valid
> Amaryllis
> 8:11
> and test that empty TE is properly checked for the empty chunk
> amc
> 8:12
> Hmmm. Maybe a virtual on CacheVConnection "setAllowEmptyContent(bool)".
> sudheerv
> 8:12
> yeah, something like that
> amc
> 8:12
> Then an implementation in CacheVC.
> sudheerv
> 8:12
> amc: i also want to make this setting overridable
> amc
> 8:12
> Then the higher layer can say "I know this was a valid zero length content, cache monkey"
> sudheerv
> 8:12
> (but, that's for later)
> 8:12
> yes, that makes sense
> Amaryllis
> 8:13
> amc: CacheVC already has allow_empty_doc flag, i think that's enough
> sudheerv
> 8:13
> Amaryllis: yeah, it does - I poked around that a bit recently, didn't seem very straightfwd
> amc
> 8:13
> Just bang on that directly?
> sudheerv
> 8:13
> but, pls go ahead and do it if you find it easy 
> Amaryllis
> 8:13
> amc: that was the plan, yes 
> 8:14
> amc: https://github.com/apache/trafficserver/pull/310/files#diff-7fada9a23fa1d12e90ca6bec876ecf8fL477
> 8:14
> amc: ... that check just needs another check for chunked as well.
> sudheerv
> 8:15
> Amaryllis: if we did something like what amc suggested above
> amc
> 8:15
> No, I meant that it would be set from (for instance) the chunked decoder or just above that.
> sudheerv
> 8:15
> where the higher layer can notify cache that it can be cached
> 8:15
> then we don't have to check any headers
> 8:15
> that already seems hacky enough
> amc
> 8:15
> So there wouldn't be an addiitional value for the config, it would just work as epexted.
> 8:15
> expected.
> sudheerv
> 8:15
> yes
> jpeach
> 8:15
> there's already a function that checks whether the whole document was received
> amc
> 8:16
> Hmmm. Interesting.
> Amaryllis
> 8:16
> i think some things are getting mixed up here - i'm not going to add an additional config value
> 8:16
> that's what the rejected PR did
> jpeach
> 8:16
> it is used when figuring what to do when getting an EOF
> amc
> 8:16
> What I would want is a way for the chunk decoder or its parent logic to pass on to the CacheVC the validity of the document.
> 8:17
> Amaryllis-  I thought your PR added the value '2' to the config variable.
> jpeach
> 8:17
> afaict as long as you can know you have the entire doc, it could be cacheable
> sudheerv
> 8:17
> jpeach: that makes sense
> Amaryllis
> 8:17
> amc: yes, but people disliked that, so instead i suggested fixing it to check for chucked encoding
> 8:17
> amc: which doesn't need an additional config setting
> sudheerv
> 8:17
> but, i guess the complication is that, at what ppint to do you know that you received the whole doc
> amc
> 8:17
> OK, I must be reading the wrong PR.
> sudheerv
> 8:17
> is it before checking allow empty doc or after
> Amaryllis
> 8:18
> amc: there's only one PR, i didn't write the chunked version yet
> sudheerv
> 8:18
> if it's just a matter of changing the order to check for whole doc, without breaking anything , then that seems like a cleaner solution
> amc
> 8:18
> Ah, OK.
> 8:19
> Yes, I agree. I think we're all on the same page - check for getting the whole document and if it's zero length and the config value is 1, cache it.
> Amaryllis
> 8:19
> (we need something for 6.0 fairly quickly, so we'll be using this patch in production, but i'm happy to put more effort into fixing it properly)
> sudheerv
> 8:19
> +1
> amc
> 8:19
> The brokeness is that complete chunked stuff isn't being checked correctly.
> 8:19
> Amaryllis - coool.
> Amaryllis
> 8:19
> yes - that's the only bit that actually needs to be fixed, but it might also make sense to refactor how the check is done
> amc
> 8:20
> Allright.
> Amaryllis
> 8:20
> i'm not sure i know enough about the TS internals to actually implement the second bit though
> amc
> 8:20
> I have to go get my Macbook fixed - update took out my wireless.
> 8:21
> I'll see if I have some time this weekend to take a look.
> sudheerv
> 8:21
> Amaryllis: if it's okay with everyone else, adding the check for TE header just to fix the TE part alone for now is fine by me as well
> 8:21
> (we can open a separate jira to improve the code for future)
> 8:22
> (was just saying, that instead of adding case by case, it'd be nicer to have a cleaner solution - but, by no means, it's immediately required)
> Amaryllis
> 8:23
> sudheerv: sure
> jpeach
> 8:23
> found it ... HttpSM::is_http_server_eos_truncation
> 8:23
> dustywusty_ is now known as dustywusty
> sudheerv
> 8:23
> jpeach: that might be a bit late?
> 8:24
> it seems to be after tunnel completes
> jpeach
> 8:24
> the place it is called from might be too late, but that's the logic for knowing if we have the whole response
> sudheerv
> 8:24
> whereas, the tunnel is writing to UA and Cache in parallel
> zwoop
> 8:24
> sorry, had to reboot.
> sudheerv
> 8:24
> jpeach: right, but, the point is
> zwoop
> 8:24
> amc sudheerv  I'm thinking, maybe we should just deprecate this setting completely ? And always allow caching empty docs when we safely can ?
> sudheerv
> 8:24
> you may get an EOS after some body is recieved already
> 8:25
> zwoop: +1 to that
> zwoop
> 8:25
> it was added for compatibility reasons eons ago, but we changed the default to "1"  in 5.x I think
> sudheerv
> 8:25
> we just need to know when is it safe 
> jpeach
> 8:25
> sudheerv: I'm just saying that we have logic to know whether we have the whole response; not that the logic is already used in all the right places
> zwoop
> 8:25
> we can deprecate it now (marking it as "don't change / use this unless you really, really have to" and remove it for 7.0.0
> sudheerv
> 8:27
> +1
> 8:27
> reveller left the room.
> zwoop
> 8:28
> sudheerv  file a Jira on it maybe ? Or two. We should change the docs now (marking it deprecated) and a bug for 7.0.0 for removal
> sudheerv
> 8:29
> sure..
> 8:30{code}



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)