Fix: [ATS 200 response after 302 not cached] (#12369)

[shongkaikai]

Fix: [ATS 9.2.x: 200 response after 302 not cached]

With backport markers:

• Project: 9.2.x
• Project: 10.x

[zwoop]

Two thoughts:

1. This doesn't seem complete, assuming it's right, since these parameters are used in several places of the HttpSM.
2. This was change ~4 years ago, and as far as I can tell, the PR here reverses the changes made then.

commit 1c3a1a41bbf90581a137505f55e06d98fe6aabe1
Author:     Damian Meden <[email protected]>
AuthorDate: Thu May 27 21:06:39 2021 +0100
Commit:     GitHub <[email protected]>
CommitDate: Thu May 27 13:06:39 2021 -0700

    Redirect - Make TS to honour the number_of_redirections configuration value (#7867)
    
    This fix includes a unit test to validate the expected behavior.

We'll have to have Damien look at this I think.

[JosiahWI]

The code change does not seem related to the issue in the title (which I will link). Why does allowing an extra redirection cycle affect whether we cache the response to any of the other redirected requests? I don't think the number of redirections should affect caching whatsoever if the concerns are properly isolated.

How have you confirmed whether or not this patch resolves your issue?

[shongkaikai]

The code change does not seem related to the issue in the title (which I will link). Why does allowing an extra redirection cycle affect whether we cache the response to any of the other redirected requests? I don't think the number of redirections should affect caching whatsoever if the concerns are properly isolated.

How have you confirmed whether or not this patch resolves your issue?

In the state_read_server_response_header(), When redirection_tries == t_state.txn_conf->number_of_redirections, enable_redirection is set to false, but it should be true at this point.

enable_redirection == false will cause s->cache_info.action = CACHE_DO_NO_ACTION in HttpTransact::handle_forward_server_connection_open, which in turn leads to failure when adding a cache consumer in perform_cache_write_action.

[brbzull0]

Hi. I think there is something missing here, this change alone may not fix all things, as this will break some other stuffs. Would it be possible to add an autest for this case?

[masaori335]

From my local test, the master and 10.1.x branch doesn't have the issue. It looks like the solution is #11542 + #12215.
@shongkaikai would you check cherry-picking #11542 and #12215 to the 9.2.x branch?

[bryancall]

This is only an issue on the 9.2.x branch and this PR is for master where the issue doesn't exist.

[bryancall]

This issue is also related: #10955. We are going to keep this issue open to track the bug and close others as duplicates.