trafficserver-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sor...@apache.org
Subject [trafficserver] 02/09: TS-4469: TS-3612 restructuring issues causing crashes in plugins. This closes #657.
Date Tue, 07 Jun 2016 16:27:45 GMT
This is an automated email from the ASF dual-hosted git repository.

sorber pushed a commit to branch 6.2.x
in repository https://git-dual.apache.org/repos/asf/trafficserver.git

commit d765dc17351f356949a3da7caafb3159c9928b9a
Author: Susan Hinrichs <shinrich@ieee.org>
AuthorDate: Mon May 23 14:33:07 2016 +0000

    TS-4469: TS-3612 restructuring issues causing crashes in plugins.  This closes #657.
    
    (cherry picked from commit 7013e90bf20a72a78a3227cb2653b69e30d4de73)
---
 proxy/http/HttpServerSession.cc  |   4 ++
 proxy/http/HttpTunnel.cc         |   7 +-
 proxy/http2/Http2ClientSession.h |   8 ++-
 proxy/http2/Http2Stream.cc       | 144 ++++++++++++++++++++++++++++-----------
 proxy/http2/Http2Stream.h        |   9 ++-
 5 files changed, 128 insertions(+), 44 deletions(-)

diff --git a/proxy/http/HttpServerSession.cc b/proxy/http/HttpServerSession.cc
index 8d0bd09..9336de3 100644
--- a/proxy/http/HttpServerSession.cc
+++ b/proxy/http/HttpServerSession.cc
@@ -180,6 +180,10 @@ HttpServerSession::release()
     return;
   }
 
+  // Make sure the vios for the current SM are cleared
+  server_vc->do_io_read(NULL, 0, NULL);
+  server_vc->do_io_write(NULL, 0, NULL);
+
   HSMresult_t r = httpSessionManager.release_session(this);
 
   if (r == HSM_RETRY) {
diff --git a/proxy/http/HttpTunnel.cc b/proxy/http/HttpTunnel.cc
index b96cc4b..f46ae67 100644
--- a/proxy/http/HttpTunnel.cc
+++ b/proxy/http/HttpTunnel.cc
@@ -1311,7 +1311,9 @@ HttpTunnel::consumer_reenable(HttpTunnelConsumer *c)
           if (is_debug_tag_set("http_tunnel"))
             Debug("http_tunnel", "Unthrottle %p %" PRId64 " / %" PRId64, p, backlog, p->backlog());
           srcp->unthrottle();
-          srcp->read_vio->reenable();
+          if (srcp->read_vio) {
+            srcp->read_vio->reenable();
+          }
           // Kick source producer to get flow ... well, flowing.
           this->producer_handler(VC_EVENT_READ_READY, srcp);
         } else {
@@ -1325,7 +1327,8 @@ HttpTunnel::consumer_reenable(HttpTunnelConsumer *c)
           }
         }
       }
-      p->read_vio->reenable();
+      if (p->read_vio)
+        p->read_vio->reenable();
     }
   }
 }
diff --git a/proxy/http2/Http2ClientSession.h b/proxy/http2/Http2ClientSession.h
index b0ddc69..48e6c84 100644
--- a/proxy/http2/Http2ClientSession.h
+++ b/proxy/http2/Http2ClientSession.h
@@ -180,7 +180,13 @@ public:
   virtual void
   release_netvc()
   {
-    client_vc = NULL;
+    // Make sure the vio's are also released to avoid
+    // later surprises in inactivity timeout
+    if (client_vc) {
+      client_vc->do_io_read(NULL, 0, NULL);
+      client_vc->do_io_write(NULL, 0, NULL);
+      client_vc = NULL;
+    }
   }
 
   sockaddr const *
diff --git a/proxy/http2/Http2Stream.cc b/proxy/http2/Http2Stream.cc
index f062b97..b2959df 100644
--- a/proxy/http2/Http2Stream.cc
+++ b/proxy/http2/Http2Stream.cc
@@ -36,9 +36,7 @@ Http2Stream::main_event_handler(int event, void *edata)
   SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
   if (e == cross_thread_event) {
     cross_thread_event = NULL;
-  }
-
-  if (e == active_event) {
+  } else if (e == active_event) {
     event = VC_EVENT_ACTIVE_TIMEOUT;
     active_event = NULL;
   } else if (e == inactive_event) {
@@ -46,6 +44,10 @@ Http2Stream::main_event_handler(int event, void *edata)
       event = VC_EVENT_INACTIVITY_TIMEOUT;
       clear_inactive_timer();
     }
+  } else if (e == read_event) {
+    read_event = NULL;
+  } else if (e == write_event) {
+    write_event = NULL;
   }
   switch (event) {
   case VC_EVENT_ACTIVE_TIMEOUT:
@@ -212,7 +214,7 @@ Http2Stream::do_io_read(Continuation *c, int64_t nbytes, MIOBuffer *buf)
 
   // Is there already data in the request_buffer?  If so, copy it over and then
   // schedule a READ_READY or READ_COMPLETE event after we return.
-  update_read_request(nbytes, true);
+  update_read_request(nbytes, false);
 
   return &read_vio;
 }
@@ -239,19 +241,27 @@ Http2Stream::do_io_write(Continuation *c, int64_t nbytes, IOBufferReader
*abuffe
 void
 Http2Stream::do_io_close(int /* flags */)
 {
+  SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
   current_reader = NULL; // SM on the way out
   if (!sent_delete) {
+    sent_delete = true;
     Debug("http2_stream", "do_io_close stream %d", this->get_id());
 
     // Only close if we are done sending data back to the client
     if (parent && (!this->is_body_done() || this->response_is_data_available()))
{
       Debug("http2_stream", "%d: Undo close to pass data", this->get_id());
-      closed = false;             // "unclose" so this gets picked up later when the netvc
side is done
-      this->reenable(&write_vio); // Kick the mechanism to get any remaining data
pushed out
-      return;
+      closed = false; // "unclose" so this gets picked up later when the netvc side is done
+      // If chunking is playing games with us, make sure we noticed when the end of message
has happened
+      if (!this->is_body_done() && this->write_vio.ndone == this->write_vio.nbytes)
{
+        this->mark_body_done();
+      } else {
+        lock.release();
+        this->reenable(&write_vio); // Kick the mechanism to get any remaining data
pushed out
+        Warning("Re-enabled to get data pushed out is_done=%d", this->is_body_done());
+        return;
+      }
     }
     closed = true;
-    sent_delete = true;
 
     if (parent) {
       // Make sure any trailing end of stream frames are sent
@@ -260,9 +270,8 @@ Http2Stream::do_io_close(int /* flags */)
     }
     parent = NULL;
 
-    SCOPED_MUTEX_LOCK(lock, this->mutex, this_ethread());
-
     clear_timers();
+    clear_io_events();
 
     if (cross_thread_event != NULL)
       cross_thread_event->cancel();
@@ -289,6 +298,7 @@ Http2Stream::initiating_close()
 
     parent = NULL;
     clear_timers();
+    clear_io_events();
 
     // This should result in do_io_close or release being called.  That will schedule the
final
     // kill yourself signal
@@ -331,8 +341,25 @@ Http2Stream::initiating_close()
   }
 }
 
+/* Replace existing event only if the new event is different than the inprogress event */
+Event *
+Http2Stream::send_tracked_event(Event *in_event, int send_event, VIO *vio)
+{
+  Event *event = in_event;
+  if (event != NULL) {
+    if (event->callback_event != send_event) {
+      event->cancel();
+      event = NULL;
+    }
+  }
+  if (event == NULL) {
+    event = this_ethread()->schedule_imm(this, send_event, vio);
+  }
+  return event;
+}
+
 void
-Http2Stream::update_read_request(int64_t read_len, bool send_update)
+Http2Stream::update_read_request(int64_t read_len, bool call_update)
 {
   if (closed || this->current_reader == NULL)
     return;
@@ -345,33 +372,43 @@ Http2Stream::update_read_request(int64_t read_len, bool send_update)
     return;
   }
   ink_release_assert(this->get_thread() == this_ethread());
-  if (send_update) {
-    SCOPED_MUTEX_LOCK(lock, read_vio.mutex, this_ethread());
-    if (read_vio.nbytes > 0 && read_vio.ndone <= read_vio.nbytes) {
-      // If this vio has a different buffer, we must copy
-      ink_release_assert(this_ethread() == this->_thread);
-      if (read_vio.buffer.writer() != (&request_buffer)) {
-        int64_t num_to_read = read_vio.nbytes - read_vio.ndone;
-        if (num_to_read > read_len)
-          num_to_read = read_len;
-        if (num_to_read > 0) {
-          int bytes_added = read_vio.buffer.writer()->write(request_reader, num_to_read);
-          if (bytes_added > 0) {
-            request_reader->consume(bytes_added);
-            read_vio.ndone += bytes_added;
-            int send_event = (read_vio.nbytes == read_vio.ndone) ? VC_EVENT_READ_COMPLETE
: VC_EVENT_READ_READY;
-            this_ethread()->schedule_imm(this, send_event, &read_vio);
-            // this->handleEvent(send_event, &read_vio);
+  SCOPED_MUTEX_LOCK(lock, read_vio.mutex, this_ethread());
+  if (read_vio.nbytes > 0 && read_vio.ndone <= read_vio.nbytes) {
+    // If this vio has a different buffer, we must copy
+    ink_release_assert(this_ethread() == this->_thread);
+    if (read_vio.buffer.writer() != (&request_buffer)) {
+      int64_t num_to_read = read_vio.nbytes - read_vio.ndone;
+      if (num_to_read > read_len)
+        num_to_read = read_len;
+      if (num_to_read > 0) {
+        int bytes_added = read_vio.buffer.writer()->write(request_reader, num_to_read);
+        if (bytes_added > 0) {
+          request_reader->consume(bytes_added);
+          read_vio.ndone += bytes_added;
+          int send_event = (read_vio.nbytes == read_vio.ndone) ? VC_EVENT_READ_COMPLETE :
VC_EVENT_READ_READY;
+          // If call_update is true, should be safe to call the read_io continuation handler
directly
+          // However, I was seeing performance regressions, so backed out this change to
track that down
+          // Probably not the cause of performance regression, but need to test some more
+          /*if (call_update) { // Safe to call vio handler directly
+            inactive_timeout_at = Thread::get_hrtime() + inactive_timeout;
+            if (read_vio._cont && this->current_reader) read_vio._cont->handleEvent(send_event,
&read_vio);
+          } else */ { // Called from do_io_read.  Still setting things up.  Send
+                                                                      // event to handle
this after the dust settles
+            read_event = send_tracked_event(read_event, send_event, &read_vio);
           }
-          ink_release_assert(!this->closed);
         }
-      } else {
-        // Try to be smart and only signal if there was additional data
-        if (request_reader->read_avail() > 0) {
-          int send_event = (read_vio.nbytes == read_vio.ndone) ? VC_EVENT_READ_COMPLETE :
VC_EVENT_READ_READY;
-          this_ethread()->schedule_imm(this, send_event, &read_vio);
-          // this->handleEvent(send_event, &read_vio);
-          ink_release_assert(!this->closed);
+      }
+    } else {
+      // Try to be smart and only signal if there was additional data
+      int send_event = (read_vio.nbytes == read_vio.ndone) ? VC_EVENT_READ_COMPLETE : VC_EVENT_READ_READY;
+      if (request_reader->read_avail() > 0 || send_event == VC_EVENT_READ_COMPLETE)
{
+        // Same comment of call_update as above
+        /*if (call_update) { // Safe to call vio handler directly
+          inactive_timeout_at = Thread::get_hrtime() + inactive_timeout;
+          if (read_vio._cont && this->current_reader) read_vio._cont->handleEvent(send_event,
&read_vio);
+        }  else */ { // Called from do_io_read.  Still setting things up.  Send event
+                                                                    // to handle this after
the dust settles
+          read_event = send_tracked_event(read_event, send_event, &read_vio);
         }
       }
     }
@@ -379,7 +416,7 @@ Http2Stream::update_read_request(int64_t read_len, bool send_update)
 }
 
 bool
-Http2Stream::update_write_request(IOBufferReader *buf_reader, int64_t write_len, bool send_update)
+Http2Stream::update_write_request(IOBufferReader *buf_reader, int64_t write_len, bool call_update)
 {
   bool retval = true;
   if (closed || parent == NULL)
@@ -436,7 +473,16 @@ Http2Stream::update_write_request(IOBufferReader *buf_reader, int64_t
write_len,
         // make sure to send the end of stream
         if (this->response_is_data_available() || send_event == VC_EVENT_WRITE_COMPLETE)
{
           if (send_event != VC_EVENT_WRITE_COMPLETE) {
-            this_ethread()->schedule_imm(this, VC_EVENT_WRITE_READY, &write_vio);
+            // As with update_read_request, should be safe to call handler directly here
if
+            // call_update is true.  Commented out for now while tracking a performance regression
+            /*if (call_update) { // Coming from reenable.  Safe to call the handler directly
+              inactive_timeout_at = Thread::get_hrtime() + inactive_timeout;
+              if (write_vio._cont && this->current_reader) write_vio._cont->handleEvent(send_event,
&write_vio);
+            } else */ { // Called from do_io_write.  Might
+                                                                                        
      // still be setting up state.  Send
+                                                                                        
      // an event to let the dust settle
+              write_event = send_tracked_event(write_event, send_event, &write_vio);
+            }
           } else {
             this->mark_body_done();
             retval = false;
@@ -460,9 +506,16 @@ Http2Stream::update_write_request(IOBufferReader *buf_reader, int64_t
write_len,
         send_response_body();
         retval = false;
       } else {
-        this_ethread()->schedule_imm(this, VC_EVENT_WRITE_READY, &write_vio);
         send_response_body();
-        // write_vio._cont->handleEvent(send_event, &write_vio);
+        // Same comment about call_update as above
+        /*if (call_update) { // Coming from reenable.  Safe to call the handler directly
+          inactive_timeout_at = Thread::get_hrtime() + inactive_timeout;
+          if (write_vio._cont && this->current_reader) write_vio._cont->handleEvent(send_event,
&write_vio);
+        } else */ { // Called from do_io_write.  Might still
+                                                                                        
  // be setting up state.  Send an event to
+                                                                                        
  // let the dust settle
+          write_event = send_tracked_event(write_event, send_event, &write_vio);
+        }
       }
     }
 
@@ -655,3 +708,14 @@ Http2Stream::clear_timers()
   clear_inactive_timer();
   clear_active_timer();
 }
+
+void
+Http2Stream::clear_io_events()
+{
+  if (read_event)
+    read_event->cancel();
+  read_event = NULL;
+  if (write_event)
+    write_event->cancel();
+  write_event = NULL;
+}
diff --git a/proxy/http2/Http2Stream.h b/proxy/http2/Http2Stream.h
index 51606f0..1008072 100644
--- a/proxy/http2/Http2Stream.h
+++ b/proxy/http2/Http2Stream.h
@@ -61,7 +61,9 @@ public:
       chunked(false),
       cross_thread_event(NULL),
       active_event(NULL),
-      inactive_event(NULL)
+      inactive_event(NULL),
+      read_event(NULL),
+      write_event(NULL)
   {
     SET_HANDLER(&Http2Stream::main_event_handler);
   }
@@ -226,8 +228,10 @@ public:
   void clear_inactive_timer();
   void clear_active_timer();
   void clear_timers();
+  void clear_io_events();
 
 private:
+  Event *send_tracked_event(Event *event, int send_event, VIO *vio);
   HTTPParser http_parser;
   ink_hrtime _start_time;
   EThread *_thread;
@@ -255,6 +259,9 @@ private:
   ink_hrtime inactive_timeout;
   ink_hrtime inactive_timeout_at;
   Event *inactive_event;
+
+  Event *read_event;
+  Event *write_event;
 };
 
 extern ClassAllocator<Http2Stream> http2StreamAllocator;

-- 
To stop receiving notification emails like this one, please contact
"commits@trafficserver.apache.org" <commits@trafficserver.apache.org>.

Mime
View raw message