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

Reapply "fetch: A beresp body cannot be BS_TAKEN" #4275

Merged
merged 2 commits into from
Feb 14, 2025

Conversation

dridi
Copy link
Member

@dridi dridi commented Feb 14, 2025

This reverts commit 8c4ae4f.

I dispute the claim from the revert commit that bo->htc->body_status
refers to the bereq, since this is managed by the V1F (HTTP/1 Fetch)
code. What we fetch on the backend side is a beresp.

The body_status can also refer to the client request.
This reverts commit 8c4ae4f.

I dispute the claim from the revert commit that bo->htc->body_status
refers to the bereq, since this is managed by the V1F (HTTP/1 Fetch)
code. What we fetch on the backend side is a beresp.
@nigoroll
Copy link
Member

Sorry, I was wrong.
It very much looks like I was under the impression of surrounding commits and simply misread the diff.

To gain confidence that I am not wrong again, I ran the test suite with this patch. It made some logexps fail, but the result is what you say:

diff --git a/bin/varnishd/cache/cache_fetch.c b/bin/varnishd/cache/cache_fetch.c
index 196817512..2cf8b5216 100644
--- a/bin/varnishd/cache/cache_fetch.c
+++ b/bin/varnishd/cache/cache_fetch.c
@@ -1115,6 +1115,12 @@ vbf_fetch_thread(struct worker *wrk, void *priv)
                AN(stp->name);
                AN(stp->func);
                stp = stp->func(wrk, bo);
+               if (bo->htc == NULL)
+                       VSLb(bo->vsl, SLT_Debug, "htc == NULL");
+               else {
+                       VSLb(bo->vsl, SLT_Debug, "htc->body_status = %s",
+                           bo->htc->body_status->name);
+               }
        }
 
        assert(bo->director_state == DIR_S_NULL);
$ find . -name \*.log | xargs grep -hE 'Debug.*htc' | sed 's:^.*Debug:Debug:'| sort | uniq -c | sort -rn
   4432 Debug           b htc == NULL
   2626 Debug           b htc->body_status = length
   1185 Debug           b htc->body_status = none
    358 Debug           b htc->body_status = chunked
     66 Debug           b htc->body_status = eof
      6 Debug          b "htc == NULL%00"                                                                                                                                                            
      4 Debug          b "htc->body_status = none%00"                                                                                                                                                
      3 Debug          "htc == NULL%00"
      3 Debug          b "htc->body_status = length%00"
      1 Debug          b "htc == NULL%00"

@nigoroll nigoroll merged commit ecddfd7 into varnishcache:master Feb 14, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants