Merge pull request apache#41 from shinrich/fix-stale-stream

Avoid using deleted stream after send data frames

Author: shinrich

proxy/http2/Http2ConnectionState.cc

@@ -1438,14 +1438,16 @@ Http2ConnectionState::send_a_data_frame(Http2Stream *stream, size_t &payload_len
 }
 
 void
-Http2ConnectionState::send_data_frames(Http2Stream *stream)
+Http2ConnectionState::send_data_frames(Http2Stream *stream, bool &stream_deleted)
 {
+  stream_deleted = false;
   // To follow RFC 7540 must not send more frames other than priority on
   // a closed stream.  So we return without sending
   if (stream->get_state() == Http2StreamState::HTTP2_STREAM_STATE_HALF_CLOSED_LOCAL ||
       stream->get_state() == Http2StreamState::HTTP2_STREAM_STATE_CLOSED) {
     Http2StreamDebug(this->ua_session, stream->get_id(), "Shutdown half closed local stream");
     this->delete_stream(stream);
+    stream_deleted = true;
     return;
   }
 
@@ -1461,6 +1463,7 @@ Http2ConnectionState::send_data_frames(Http2Stream *stream)
       // See 'closed' state written at [RFC 7540] 5.1.
       Http2StreamDebug(this->ua_session, stream->get_id(), "Shutdown stream");
       this->delete_stream(stream);
+      stream_deleted = true;
     }
   }
 

proxy/http2/Http2ConnectionState.h

@@ -257,7 +257,7 @@ class Http2ConnectionState : public Continuation
   // HTTP/2 frame sender
   void schedule_stream(Http2Stream *stream);
   void send_data_frames_depends_on_priority();
-  void send_data_frames(Http2Stream *stream);
+  void send_data_frames(Http2Stream *stream, bool &stream_deleted);
   Http2SendDataFrameResult send_a_data_frame(Http2Stream *stream, size_t &payload_length);
   void send_headers_frame(Http2Stream *stream);
   void send_push_promise_frame(Http2Stream *stream, URL &url, const MIMEField *accept_encoding);

proxy/http2/Http2Stream.cc

@@ -328,15 +328,16 @@ Http2Stream::do_io_close(int /* flags */)
     // by the time this is called from transaction_done.
     closed = true;
 
+    clear_timers();
+    clear_io_events();
+
+    bool stream_deleted = false;
     if (parent && this->is_client_state_writeable()) {
       // Make sure any trailing end of stream frames are sent
       // Wee will be removed at send_data_frames or closing connection phase
-      static_cast<Http2ClientSession *>(parent)->connection_state.send_data_frames(this);
+      static_cast<Http2ClientSession *>(parent)->connection_state.send_data_frames(this, stream_deleted);
     }
 
-    clear_timers();
-    clear_io_events();
-
     // Wait until transaction_done is called from HttpSM to signal that the TXN_CLOSE hook has been executed
   }
 }
@@ -517,8 +518,9 @@ Http2Stream::update_read_request(int64_t read_len, bool call_update, bool check_
 void
 Http2Stream::restart_sending()
 {
-  send_response_body();
-  if (this->write_vio.ntodo() > 0 && this->write_vio.get_writer()->write_avail() > 0) {
+  bool stream_deleted;
+  send_response_body(stream_deleted);
+  if (!stream_deleted && this->write_vio.ntodo() > 0 && this->write_vio.get_writer()->write_avail() > 0) {
     write_vio._cont->handleEvent(VC_EVENT_WRITE_READY, &write_vio);
   }
 }
@@ -569,6 +571,7 @@ Http2Stream::update_write_request(IOBufferReader *buf_reader, int64_t write_len,
                    write_vio.nbytes, write_vio.ndone, write_vio.get_writer()->write_avail(), bytes_avail);
 
   if (bytes_avail > 0 || is_done) {
+    bool stream_deleted;
     int send_event = (write_vio.ntodo() == bytes_avail || is_done) ? VC_EVENT_WRITE_COMPLETE : VC_EVENT_WRITE_READY;
 
     // Process the new data
@@ -613,18 +616,20 @@ 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) {
-            send_response_body();
-            // 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
-              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);
+            send_response_body(stream_deleted);
+            if (!stream_deleted) {
+              // 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
+                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();
-            send_response_body();
+            send_response_body(stream_deleted);
           }
         }
         break;
@@ -640,15 +645,17 @@ Http2Stream::update_write_request(IOBufferReader *buf_reader, int64_t write_len,
         // Defer sending the write complete until the send_data_frame has sent it all
         // this_ethread()->schedule_imm(this, send_event, &write_vio);
         this->mark_body_done();
-        send_response_body();
+        send_response_body(stream_deleted);
         retval = false;
       } else {
-        send_response_body();
-        if (call_update) { // Coming from reenable.  Safe to call the handler directly
-          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);
+        send_response_body(stream_deleted);
+        if (!stream_deleted) {
+          if (call_update) { // Coming from reenable.  Safe to call the handler directly
+            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);
+          }
         }
       }
     }
@@ -667,18 +674,20 @@ Http2Stream::push_promise(URL &url, const MIMEField *accept_encoding)
 }
 
 void
-Http2Stream::send_response_body()
+Http2Stream::send_response_body(bool &stream_deleted)
 {
+  stream_deleted = false;
   Http2ClientSession *parent = static_cast<Http2ClientSession *>(this->get_parent());
 
+  inactive_timeout_at = Thread::get_hrtime() + inactive_timeout;
+
   if (Http2::stream_priority_enabled) {
     SCOPED_MUTEX_LOCK(lock, parent->connection_state.mutex, this_ethread());
     parent->connection_state.schedule_stream(this);
   } else {
     SCOPED_MUTEX_LOCK(lock, parent->connection_state.mutex, this_ethread());
-    parent->connection_state.send_data_frames(this);
+    parent->connection_state.send_data_frames(this, stream_deleted);
   }
-  inactive_timeout_at = Thread::get_hrtime() + inactive_timeout;
 }
 
 void

proxy/http2/Http2Stream.h

@@ -189,7 +189,7 @@ class Http2Stream : public ProxyClientTransaction
   void transaction_done() override;
 
   void restart_sending();
-  void send_response_body();
+  void send_response_body(bool &stream_deleted);
   void push_promise(URL &url, const MIMEField *accept_encoding);
 
   // Stream level window size