TS-4396: fix number_of_redirections off-by-one

[scw00]

The enable_redirection will be turned off in the HttpSM::state_read_server_response_header

    if (enable_redirection && (redirection_tries < t_state.txn_conf->number_of_redirections)) {
      ++redirection_tries;
    } else {
      tunnel.deallocate_redirect_postdata_buffers();
      enable_redirection = false;
    }

The redirect data is never cached if number_of_redirections is one.

 if (s->redirect_info.redirect_in_process && s->state_machine->enable_redirection) {
    if (s->cache_info.action == CACHE_DO_NO_ACTION) {
      switch (s->hdr_info.server_response.status_get()) {
      case HTTP_STATUS_MULTIPLE_CHOICES:   // 300
      case HTTP_STATUS_MOVED_PERMANENTLY:  // 301
      case HTTP_STATUS_MOVED_TEMPORARILY:  // 302
      case HTTP_STATUS_SEE_OTHER:          // 303
      case HTTP_STATUS_USE_PROXY:          // 305
      case HTTP_STATUS_TEMPORARY_REDIRECT: // 307
        break;
      default:
        DebugTxn("http_trans", "[hfsco] redirect in progress, non-3xx response, setting cache_do_write");
        if (cw_vc && s->txn_conf->cache_http) {
          s->cache_info.action = CACHE_DO_WRITE;
        }
        break;
      }
    }
  }

#786
@mingzym Please review

[scw00]

golang script

package main

import (
	"github.com/julienschmidt/httprouter"
	"net/http"
	"github.com/qiniu/log"
)

var  router  *httprouter.Router

func createData(w http.ResponseWriter, r *http.Request, p httprouter.Params)  {
	log.Warn("recieve request!! size: ", r.Header.Get("size"), r.URL.String(), r.Method)

	w.Header().Add("Location", "http://127.0.0.1:12346/index.html")

	http.Error(w, http.StatusText(302), 302)
	return
}

func createData6(w http.ResponseWriter, r *http.Request, p httprouter.Params)  {
	log.Warn("recieve request!! size: ", r.Header.Get("size"), r.URL.String(), r.Method)

	w.Header().Add("Cache-Control", "max-age=10")
	w.Write([]byte("hello world"))
//  http.Error(w, http.StatusText(404), 404)
	return
}

func port12345() {
	router = httprouter.New();
	router.GET("/*<num>", createData)
	router.POST("/*<num>", createData)

	http.ListenAndServe(":12345", router)
}

func port12346() {
	router = httprouter.New();
	router.GET("/*<num>", createData6)
	router.POST("/*<num>", createData6)

	http.ListenAndServe(":12346", router)
}

func main()  {
	go port12345()
	port12346()
}

[scw00]

By the way, the enable_redirection could be removed and use "redirection_tries < t_state.txn_conf->number_of_redirections" instead.

[mingzym]

[approve ci]
looks good to me, I'd advise that you create separated case(pr) for the test case and we can always merge the test case first.

[scw00]

That looks better.

[mingzym]

looks good to me
[approve ci]

[bryancall]

[approve ci]

[mingzym]

hmm, crash on transform test, should do nothing with this pr, but how can we debug that crash on the build if we got a core on the building test.

[zwoop]

[approve ci linux]

[zwoop]

@oknet @scw00 Is this a candidate for 7.1.0 ? If so, please change Milestone and add Label Backport.

[scw00]

Yes, it is candidate for 7.1.0 .

[zwoop]

@oknet can you review / approve this as well please?

[scw00]

@oknet please review!

[zwoop]

[approve ci]

[oknet]

@zwoop Can you comment the below changes in dfc2a8f:

commit dfc2a8f8dba53cd0ab541753ac192eb06bba2fa4
Author: Leif Hedstrom <zwoop@apache.org>
Date:   Wed Apr 2 10:21:57 2014 -0600

    TS-2691 Fix how we count redirect retries
    
    TS-2691 Also eliminate api_enable_redirection which seems superfluous

......
@@ -1837,11 +1832,9 @@ HttpSM::state_read_server_response_header(int event, void *data)
     t_state.transact_return_point = HttpTransact::HandleResponse;
     t_state.api_next_action = HttpTransact::SM_ACTION_API_READ_RESPONSE_HDR;
 
-    // YTS Team, yamsat Plugin
-    // Incrementing redirection_tries according to config parameter
     // if exceeded limit deallocate postdata buffers and disable redirection
-    if (enable_redirection && (redirection_tries <= HttpConfig::m_master.number_of_redirections)) {
-      redirection_tries++;
+    if (enable_redirection && (redirection_tries < HttpConfig::m_master.number_of_redirections)) {
+      ++redirection_tries;
     } else {
       tunnel.deallocate_redirect_postdata_buffers();
       enable_redirection = false;
@@ -7267,12 +7260,7 @@ void
 HttpSM::do_redirect()
 {
   DebugSM("http_redirect", "[HttpSM::do_redirect]");
-  if (enable_redirection == false || redirection_tries > (HttpConfig::m_master.number_of_redirections)) {
-    tunnel.deallocate_redirect_postdata_buffers();
-    return;
-  }
-
-  if (api_enable_redirection == false) {
+  if (!enable_redirection || redirection_tries >= HttpConfig::m_master.number_of_redirections) {
     tunnel.deallocate_redirect_postdata_buffers();
     return;
   }
......

I think these changes should be revert as @scw00 does in this PR.

[zwoop]

@oknet I have no recollections / memory of this :-). Does this PR revert the changes as needed, or do we need a separate PR to revert? I have no objections to either.

[zwoop]

If we need to revert something, which is not covered here, please make a PR of course.

[oknet]

@zwoop only revert these 2 lines not all the commit dfc2a8f.

Since the JIRA is not working now, I can not open the TS-2691 and find the reason of the changes.

Let's hold this PR and wait for the JIRA back.

[oknet]

The commit dfc2a8f for https://issues.apache.org/jira/browse/TS-2691 change the range of redirection_tries from [1, max) to [0, max).

The redirection_tries is set to 0 if redirect is disabled.
The redirection_tries is >= 1 if redirect is enabled and it indicate the times of following the 3xx redirection.

I think we should not change the conditions in TS-2691. Just replace the enable_redirection == false with redirection_tries == 0.

[oknet]

@zwoop The below changes in dfc2a8f, Does it means if redirect enabled and this is the first follow on redirect ... ?

With the changes, It means every follow on redirect ...

@@ -903,8 +903,7 @@ HttpTunnel::producer_run(HttpTunnelProducer * p)
   //YTS Team, yamsat Plugin
   // Allocate and copy partial POST data to buffers. Check for the various parameters
   // including the maximum configured post data size
-  if (p->alive && sm->t_state.method == HTTP_WKSIDX_POST && sm->enable_redirection
-      && sm->redirection_tries == 0 && (p->vc_type == HT_HTTP_CLIENT)) {
+  if (p->alive && sm->t_state.method == HTTP_WKSIDX_POST && sm->enable_redirection && (p->vc_type == HT_HTTP_CLIENT)) {
     Debug("http_redirect", "[HttpTunnel::producer_run] client post: %" PRId64" max size: %" PRId64"",
           p->buffer_start->read_avail(), HttpConfig::m_master.post_copy_size);

[scw00]

Hmm. I think, we need to produce post data in every request.

We setup a STATIC PRODUCER during redirecting, and free post buffer.
So, we need to save the post data again if we need more redirection.

// do_setup_post_tunnel
  if (t_state.redirect_info.redirect_in_process && enable_redirection &&
      (tunnel.postbuf && tunnel.postbuf->postdata_copy_buffer_start != nullptr &&
       tunnel.postbuf->postdata_producer_buffer != nullptr)) {
    post_redirect = true;
    // copy the post data into a new producer buffer for static producer
    tunnel.postbuf->postdata_producer_buffer->write(tunnel.postbuf->postdata_copy_buffer_start);
    post_bytes       = tunnel.postbuf->postdata_producer_reader->read_avail();
    transfered_bytes = post_bytes;
    p                = tunnel.add_producer(HTTP_TUNNEL_STATIC_PRODUCER, post_bytes, tunnel.postbuf->postdata_producer_reader,
                            (HttpProducerHandler) nullptr, HT_STATIC, "redirect static agent post");
    // the tunnel has taken over the buffer and will free it
    tunnel.postbuf->postdata_producer_buffer = nullptr;
    tunnel.postbuf->postdata_producer_reader = nullptr;
  } else {

[oknet]

@scw00 Yes, we should collect the post payload into postbuf for every CLIENT_VC as a producer.

[zwoop]

We want this on 7.1.0 still, right ?

[scw00]

yes, it is!!

[zwoop]

Cherry-picked to 7.1.0