Move the postbuf into HttpSM by scw00 · Pull Request #2766 · apache/trafficserver

scw00 · 2017-11-04T04:06:06Z

Move the postbuf into HttpSM.

scw00 · 2017-11-04T09:38:24Z

proxy/http/HttpSM.cc

  tunnel.reset();

-  tunnel.allocate_redirect_postdata_producer_buffer();
-


This line will cause the post buf truncated.

We should not deallocate postbuf. postdata_copy_buffer_start because it is the same as we doing postbuf.ua_buffer_reader->consume.

oknet · 2017-11-04T09:39:41Z

iocore/eventsystem/I_IOBuffer.h

 #define DEFAULT_BUFFER_NUMBER 128
 #define DEFAULT_HUGE_BUFFER_NUMBER 32
-#define MAX_MIOBUFFER_READERS 5
+#define MAX_MIOBUFFER_READERS 6


IMO, the max value 5 is enough.

oknet · 2017-11-04T10:00:42Z

proxy/http/HttpSM.cc

    // copy the post data into a new producer buffer for static producer
-    tunnel.postbuf->postdata_producer_buffer->write(tunnel.postbuf->postdata_copy_buffer_start);
-    int64_t post_bytes = tunnel.postbuf->postdata_producer_reader->read_avail();
+    MIOBuffer *postdata_producer_buffer      = new_empty_MIOBuffer();


It saves the memory to avoid allocate a new IOBufferBlock because it always get data by clone().

oknet · 2017-11-04T10:03:04Z

proxy/http/HttpSM.cc

+
+  if (this->postdata_copy_buffer == nullptr) {
+    ink_assert(this->postdata_copy_buffer_start == nullptr);
+    this->postdata_copy_buffer       = new_MIOBuffer(BUFFER_SIZE_INDEX_4K);


IMO, new_empty_MIOBuffer() is more better, the same as postdata_producer_buffer above.

oknet · 2017-11-04T11:18:46Z

The PostDataBuffers is designed to save the post payload while retry to make a new request.

The HttpTunnel is designed to move the data from producer to one or more consumers. It should not keep any data after tunnel.reset().

Any data that is received from ua_session or server_session, the owner of these data is HttpSM.

According to the above analysis, the PR moves the PostDataBuffers from HttpTunnel to HttpSM.

With the PR,

Incoming data is saved into the PostDataBuffers by HttpTunnel
Tunnel is reset if request is redirected. But the HttpSM still keeps the post payload.
HttpSM make a new request and clone the post payload for new HttpTunnel.

d2r · 2017-11-04T15:40:35Z

proxy/http/HttpSM.h

+HttpSM::postbuf_read_ndone()
+{
+  return this->_postbuf.postdata_copy_buffer_start->read_avail();
+}


Could we rename this method to something like postbuf_buffer_avail?

Same reasoning as above suggestion to rename postbuf_read_avail.

As the comment above!

d2r · 2017-11-04T16:03:14Z

proxy/http/HttpSM.cc

  DebugSM("http_redirect", "[HttpSM::do_redirect]");
  if (!enable_redirection || redirection_tries > t_state.txn_conf->number_of_redirections) {
-    tunnel.deallocate_redirect_postdata_buffers();
+    // tunnel.deallocate_redirect_postdata_buffers();


We can remove the comment.

d2r · 2017-11-04T16:05:32Z

proxy/http/HttpSM.cc

+  }
+
+  if (this->ua_buffer_reader != nullptr) {
+    this->ua_buffer_reader->mbuf->dealloc_reader(this->ua_buffer_reader);


Do we need to check that this->ua_buffer_reader->mbuf != nullptr?

Yes, the ua_buffer_reader may be released in tunnel.

d2r · 2017-11-04T16:12:13Z

proxy/http/HttpSM.cc

+{
+  // the this->ua_buffer_reader may be realeased in tunnel::deallocate_buffers.
+  // we always set a new reader instead of the old value;
+  // ink_assert(this->ua_buffer_reader == nullptr);


Probably we should remove this comment and guarantee that this->ua_buffer_reader != nullptr before we return.

The following assume it is not null:

PostDataBuffers::copy_partial_post_data

HttpSM::postbuf_read_avail

Sounds reasonable !

d2r · 2017-11-04T16:34:13Z

proxy/http/HttpSM.h

+HttpSM::postbuf_read_avail()
+{
+  return this->_postbuf.ua_buffer_reader->read_avail();
+}


Could we rename this method to something like postbuf_reader_avail?

Referencing a debug log message in HttpTunnel.cc:

1163 Debug("http_redirect", "[HttpTunnel::producer_handler] post exceeds buffer limit, buffer_avail=%" PRId64 1164 " reader_avail=%" PRId64 " limit=%" PRId64 "", 1165 sm->postbuf_read_ndone(), sm->postbuf_read_avail(), HttpConfig::m_master.post_copy_size);

Here the message uses "buffer_avail" and "reader_avail", and it seems clearer.

good point ! I'll fix that .

d2r · 2017-11-04T16:41:58Z

proxy/http/HttpSM.h

+  void copy_partial_post_data();
+
+  ~PostDataBuffers();
+};


Just an observation. By omitting the default constructor definition here, we are losing the following debug message from HttpTunnel.h

266 Debug("http_redirect", "[PostDataBuffers::PostDataBuffers]");

This may not have been a very useful debug message though.

IMO, postbuf in HttpSM is not a pointer. It is always allocated with HttpSM.

OK, sounds good. Just checking.

d2r

👍 (non-binding)

zwoop · 2017-11-08T17:08:37Z

Candidate for 7.1.2 ?

bryancall · 2017-11-08T17:13:20Z

@scw00
We should have an autest for this to make sure POSTs work with redirection.

scw00 · 2017-11-08T23:57:49Z

@bryancall . I'll give another pr on this weekends.

scw00 · 2017-11-09T00:17:18Z

@zwoop! Yes it is candidate for 7.1.2, but please waiting for my autest pr.

zwoop · 2017-11-10T05:22:18Z

Alrighty, let me know what other PRs this depends on when ready.

d2r · 2017-11-27T15:08:42Z

@scw00 #2800 was merged. Do we want to back-port this?

d2r · 2017-12-06T14:36:47Z

@scw00 @zwoop Ping

scw00 · 2017-12-07T00:11:44Z

Yes we can port this pr to 7.1.x @zwoop now.

zwoop · 2018-01-22T23:30:40Z

Cherry-picked to 7.1.3

scw00 force-pushed the redirect_post_buffer branch 6 times, most recently from b02d00e to 961b379 Compare November 4, 2017 08:38

scw00 commented Nov 4, 2017

View reviewed changes

scw00 force-pushed the redirect_post_buffer branch from 961b379 to 24e044f Compare November 4, 2017 10:10

oknet requested changes Nov 4, 2017

View reviewed changes

oknet assigned scw00 Nov 4, 2017

oknet added Core HTTP labels Nov 4, 2017

oknet added this to the 8.0.0 milestone Nov 4, 2017

scw00 force-pushed the redirect_post_buffer branch from 24e044f to bd84fd9 Compare November 4, 2017 10:52

oknet previously approved these changes Nov 4, 2017

View reviewed changes

d2r mentioned this pull request Nov 4, 2017

fix #2677: Issue with null ua_buffer_reader due to PR #2123 #2692

Closed

d2r reviewed Nov 4, 2017

View reviewed changes

scw00 dismissed oknet’s stale review via acbbe9a November 5, 2017 02:14

scw00 force-pushed the redirect_post_buffer branch from bd84fd9 to acbbe9a Compare November 5, 2017 02:14

move the postbuf to HttpSM

daafeda

scw00 force-pushed the redirect_post_buffer branch from acbbe9a to daafeda Compare November 5, 2017 06:32

oknet approved these changes Nov 6, 2017

View reviewed changes

oknet mentioned this pull request Nov 6, 2017

TS-4042: Add feature to buffer request body before making downstream requests #2335

Merged

d2r approved these changes Nov 6, 2017

View reviewed changes

zwoop merged commit b9988fd into apache:master Nov 8, 2017

This was referenced Nov 12, 2017

add autest for post redirection #2800

Merged

The post data will be truncated when we do redirection twice. #2693

Closed

Issue with null ua_buffer_reader due to PR #2123 #2677

Closed

zizhong mentioned this pull request Nov 28, 2017

HttpRedirect doesn't work with big post request #2869

Closed

zwoop modified the milestones: 8.0.0, 7.1.3 Jan 22, 2018

scw00 deleted the redirect_post_buffer branch March 15, 2018 06:29

		tunnel.reset();

		tunnel.allocate_redirect_postdata_producer_buffer();

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oknet commented Nov 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

d2r left a comment

Choose a reason for hiding this comment

Uh oh!

zwoop commented Nov 8, 2017

Uh oh!

bryancall commented Nov 8, 2017

Uh oh!

scw00 commented Nov 8, 2017

Uh oh!

scw00 commented Nov 9, 2017

Uh oh!

zwoop commented Nov 10, 2017

Uh oh!

d2r commented Nov 27, 2017

Uh oh!

d2r commented Dec 6, 2017

Uh oh!

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

Uh oh!

Uh oh!

zwoop commented Jan 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

scw00 commented Nov 4, 2017 •

edited

Loading

scw00 Nov 4, 2017 •

edited

Loading

scw00 Nov 5, 2017 •

edited

Loading

scw00 Nov 5, 2017 •

edited

Loading

scw00 commented Dec 7, 2017 •

edited

Loading