TS-4396: fix number_of_redirections off-by-one #2092

scw00 · 2017-06-08T01:07:13Z

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 · 2017-06-08T01:08:06Z

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 · 2017-06-08T01:13:16Z

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

mingzym · 2017-06-08T01:48:26Z

[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 · 2017-06-08T01:55:10Z

That looks better.

mingzym

looks good to me
[approve ci]

bryancall · 2017-06-08T02:37:21Z

[approve ci]

mingzym · 2017-06-08T04:14:24Z

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 · 2017-06-08T17:19:22Z

[approve ci linux]

zwoop · 2017-06-08T20:39:23Z

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

scw00 · 2017-06-09T03:54:17Z

Yes, it is candidate for 7.1.0 .

zwoop · 2017-06-09T20:53:44Z

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

scw00 · 2017-06-13T07:20:00Z

@oknet please review!

zwoop · 2017-06-13T10:12:05Z

[approve ci]

oknet · 2017-06-13T10:23:53Z

@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 · 2017-06-13T10:29:42Z

@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 · 2017-06-13T10:31:22Z

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

oknet · 2017-06-13T11:23:23Z

@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 · 2017-06-13T14:33:18Z

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 · 2017-06-13T14:54:00Z

@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 · 2017-06-14T00:41:17Z

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 · 2017-06-14T08:08:06Z

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

zwoop · 2017-06-18T15:06:56Z

We want this on 7.1.0 still, right ?

scw00 · 2017-06-18T22:14:59Z

yes, it is!!

zwoop · 2017-06-21T22:03:57Z

Cherry-picked to 7.1.0

mingzym previously approved these changes Jun 8, 2017

View reviewed changes

bryancall assigned scw00 Jun 8, 2017

bryancall added the HTTP label Jun 8, 2017

zwoop self-requested a review June 8, 2017 17:19

zwoop added this to the 8.0.0 milestone Jun 8, 2017

bryancall mentioned this pull request Jun 8, 2017

TS-4396: fix number_of_redirections off-by-one #786

Closed

oknet added the Backport Marked for backport for an LTS patch release label Jun 9, 2017

oknet modified the milestones: 7.1.0, 8.0.0 Jun 9, 2017

scw00 dismissed mingzym’s stale review via 3c5c0f8 June 13, 2017 07:10

TS-4396: fix number_of_redirections off-by-one

88cf9d8

apache deleted a comment from oknet Jun 13, 2017

oknet approved these changes Jun 14, 2017

View reviewed changes

oknet merged commit 452a2dc into apache:master Jun 17, 2017

scw00 deleted the redierct_301_cache branch June 19, 2017 02:12

TS-4396: fix number_of_redirections off-by-one #2092

TS-4396: fix number_of_redirections off-by-one #2092

Uh oh!

Conversation

scw00 commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scw00 commented Jun 8, 2017

Uh oh!

scw00 commented Jun 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mingzym commented Jun 8, 2017

Uh oh!

scw00 commented Jun 8, 2017

Uh oh!

mingzym left a comment

Choose a reason for hiding this comment

Uh oh!

bryancall commented Jun 8, 2017

Uh oh!

mingzym commented Jun 8, 2017

Uh oh!

zwoop commented Jun 8, 2017

Uh oh!

zwoop commented Jun 8, 2017

Uh oh!

scw00 commented Jun 9, 2017

Uh oh!

zwoop commented Jun 9, 2017

Uh oh!

scw00 commented Jun 13, 2017

Uh oh!

zwoop commented Jun 13, 2017

Uh oh!

oknet commented Jun 13, 2017

Uh oh!

zwoop commented Jun 13, 2017

Uh oh!

zwoop commented Jun 13, 2017

Uh oh!

oknet commented Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oknet commented Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oknet commented Jun 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scw00 commented Jun 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oknet commented Jun 14, 2017

Uh oh!

zwoop commented Jun 18, 2017

Uh oh!

scw00 commented Jun 18, 2017

Uh oh!

zwoop commented Jun 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

scw00 commented Jun 8, 2017 •

edited

Loading

scw00 commented Jun 8, 2017 •

edited

Loading

oknet commented Jun 13, 2017 •

edited

Loading

oknet commented Jun 13, 2017 •

edited

Loading

oknet commented Jun 13, 2017 •

edited

Loading

scw00 commented Jun 14, 2017 •

edited

Loading