Revert D61436491

Summary: This diff reverts D61436491 Broke a bunch of tests Reviewed By: afrind Differential Revision: D62545890 fbshipit-source-id: 1c3fe7a25341c2d6468c47fdb16fba9f307b8675
facebook · Sep 12, 2024 · df4d148 · df4d148
1 parent db3aa98
commit df4d148
Show file tree

Hide file tree

Showing 4 changed files with 24 additions and 141 deletions.
diff --git a/third-party/proxygen/src/proxygen/lib/http/session/HTTPSession.cpp b/third-party/proxygen/src/proxygen/lib/http/session/HTTPSession.cpp
@@ -259,19 +259,6 @@ HTTPSession::~HTTPSession() {
   runDestroyCallbacks();
 }
 
-std::chrono::milliseconds HTTPSession::getDrainTimeout() const {
-  static constexpr std::chrono::milliseconds kDefaultDrainTimeout{
-      std::chrono::seconds(5)};
-  auto controller = getController();
-  if (controller) {
-    auto controllerTimeout = controller->getGracefulShutdownTimeout();
-    if (controllerTimeout < kDefaultDrainTimeout) {
-      return controllerTimeout;
-    }
-  }
-  return kDefaultDrainTimeout;
-}
-
 void HTTPSession::startNow() {
   CHECK(!started_);
   started_ = true;
@@ -285,13 +272,11 @@ void HTTPSession::startNow() {
   // util we've started and sent SETTINGS.
   if (draining_) {
     codec_->generateGoaway(writeBuf_);
-    if (codec_->isWaitingToDrain()) {
-      wheelTimer_.scheduleTimeout(&drainTimeout_, getDrainTimeout());
-    } else if (isDownstream()) {
-      // transactions must be empty and reads cannot be shutdown
-      VLOG(4) << "Starting drain timer";
-      resetTimeoutTo(getDrainTimeout());
-    } // it's upstream, let the client close it when they want
+    auto controller = getController();
+    if (controller && codec_->isWaitingToDrain()) {
+      wheelTimer_.scheduleTimeout(&drainTimeout_,
+                                  controller->getGracefulShutdownTimeout());
+    }
   }
   scheduleWrite();
   resumeReads();
@@ -370,17 +355,7 @@ void HTTPSession::readTimeoutExpired() noexcept {
 
   DestructorGuard g(this);
   setCloseReason(ConnectionCloseReason::TIMEOUT);
-  if (!codec_->isReusable() && transactions_.empty()) {
-    LOG_IF(DFATAL, readsShutdown()) << "Why did we have a read timer running?";
-    // Shutdown reads (uninstall read callback, etc).  Session will close
-    VLOG(4) << "Shutdown from readTimeoutExpired sess=" << *this;
-    shutdownTransport(true, false);
-  } // otherwise
-  //    1. The codec is re-usable - we will notifyPendingShutdown to start drain
-  //    2. There's an active txn; the FIN timer will start from
-  //         onEgressMessageFinished/ShutdownTransportCallback.
   notifyPendingShutdown();
-  checkForShutdown();
 }
 
 void HTTPSession::writeTimeoutExpired() noexcept {
@@ -2013,12 +1988,8 @@ void HTTPSession::detach(HTTPTransaction* txn) noexcept {
     }
   }
 
-  if (liveTransactions_ == 0 && transactions_.empty() && !isScheduled() &&
-      codec_->isReusable()) {
-    // Start the idle timer again
+  if (liveTransactions_ == 0 && transactions_.empty() && !isScheduled()) {
     resetTimeout();
-    // if transactions empty and codec is not re-usable, fin timeout scheduled
-    // from onEgressMessageFinished/ShutdownTransportCallback
   }
 
   // It's possible that this is the last transaction in the session,
@@ -2415,9 +2386,7 @@ void HTTPSession::shutdownTransport(bool shutdownReads,
         shutdownReads = true;
       } else {
         VLOG(4) << *this << " writes drained, closing";
-        if (isUpstream() || !codec_->supportsParallelRequests()) {
-          sock_->shutdownWriteNow();
-        }
+        sock_->shutdownWriteNow();
       }
       notifyEgressShutdown = true;
     } else if (!writesDraining_) {
@@ -2538,8 +2507,7 @@ void HTTPSession::checkForShutdown() {
   // Two conditions are required to destroy the HTTPSession:
   //   * All writes have been finished.
   //   * There are no transactions remaining on the session.
-  if (writesShutdown() && transactions_.empty() && !isLoopCallbackScheduled() &&
-      (isUpstream() || readsShutdown())) {
+  if (writesShutdown() && transactions_.empty() && !isLoopCallbackScheduled()) {
     VLOG(4) << "destroying " << *this;
     shutdownRead();
     auto asyncSocket = sock_->getUnderlyingTransport<folly::AsyncSocket>();
@@ -2584,18 +2552,11 @@ void HTTPSession::drainImpl() {
     if (codec_->generateGoaway(writeBuf_) > 0) {
       scheduleWrite();
     }
-    if (codec_->isWaitingToDrain()) {
-      // Schedule another goaway
-      wheelTimer_.scheduleTimeout(&drainTimeout_, getDrainTimeout());
-    } else if (transactions_.empty() && isDownstream() && !readsShutdown()) {
-      // Codec is not reusable, but we need to wait for peer FIN
-      VLOG(4) << "Starting drain timer sess=" << *this;
-      resetTimeoutTo(getDrainTimeout());
-    } // else
-    //   1. there's an txn - FIN timeout set from
-    //       onEgressMessageFinished/ShutdownTransportCallback
-    //   2. Client session - let don't need to wait for peer FIN
-    //   3. reads were already shutdown, nothing to wait for
+    auto controller = getController();
+    if (controller && codec_->isWaitingToDrain()) {
+      wheelTimer_.scheduleTimeout(&drainTimeout_,
+                                  controller->getGracefulShutdownTimeout());
+    }
   }
 }
 
@@ -2865,8 +2826,7 @@ void HTTPSession::writeSuccess() noexcept {
       setCloseReason(ConnectionCloseReason::UNKNOWN);
     }
     VLOG(4) << *this << " shutdown from onWriteSuccess";
-    // downstream needs to listen for fin to shutdown reads
-    shutdownTransport(isUpstream(), true);
+    shutdownTransport(true, true);
   }
   numActiveWrites_--;
   if (!inLoopCallback_) {

diff --git a/third-party/proxygen/src/proxygen/lib/http/session/HTTPSession.h b/third-party/proxygen/src/proxygen/lib/http/session/HTTPSession.h
@@ -1000,13 +1000,8 @@ class HTTPSession
 
     void runLoopCallback() noexcept override {
       VLOG(4) << *session_ << " shutdown from onEgressMessageFinished";
-      bool shutdownReads = session_->ingressError_;
-      if (session_->isDownstream() && !session_->readsShutdown() &&
-          !shutdownReads) {
-        auto to = session_->getDrainTimeout();
-        VLOG(4) << "Starting drain timer t=" << to.count();
-        session_->resetTimeoutTo(to);
-      }
+      bool shutdownReads =
+          session_->isDownstream() && !session_->ingressUpgraded_;
       auto dg = dg_.release();
       session_->shutdownTransport(shutdownReads, true);
       delete dg;
@@ -1058,7 +1053,6 @@ class HTTPSession
     HTTPSession* session_;
   };
   DrainTimeout drainTimeout_;
-  std::chrono::milliseconds getDrainTimeout() const;
 
   class PingProber : public folly::HHWheelTimer::Callback {
    public: