Don't detach handler after onError

Summary:
I believe onError results in destruction of the sink, like how onEOM can/does.

HTTPMessageFilters don't detachTransaction():
```
void onError(const HTTPException& error) noexcept override {
    nextTransactionHandler_->onError(error);
  }
```
And, more importantly: Got a segfault when testing in traflab:
```
*** Aborted at 1728517165 (Unix time, try 'date -d 1728517165') ***
*** Signal 11 (SIGSEGV) (0x0) received by PID 929 (pthread TID 0x7fafd67ff640) (linux TID 1030) (code: address not mapped to object), stack trace: ***
    @ 000000000e5e755a folly::symbolizer::(anonymous namespace)::innerSignalHandler(int, siginfo_t*, void*) [clone .__uniq.302291754384189453301783370447166124111]
                       ./fbcode/folly/debugging/symbolizer/SignalHandler.cpp:453
    @ 000000000e5e749e folly::symbolizer::(anonymous namespace)::signalHandler(int, siginfo_t*, void*) [clone .__uniq.302291754384189453301783370447166124111] [clone .llvm.12006849344459385367]
                       ./fbcode/folly/debugging/symbolizer/SignalHandler.cpp:474
    @ 000000000004455f (unknown)
                       /home/engshare/third-party2/glibc/2.34/src/glibc-2.34/signal/../sysdeps/unix/sysv/linux/libc_sigaction.c:8
                       -> /home/engshare/third-party2/glibc/2.34/src/glibc-2.34/signal/../sysdeps/unix/sysv/linux/x86_64/libc_sigaction.c
    @ 0000000011e8046d proxygen::HTTPTunnelSink::readErr(folly::AsyncSocketException const&)
                       ./fbcode/proxygen/lib/http/sink/HTTPTunnelSink.cpp:141
```

Reviewed By: jalopezsilva

Differential Revision: D64148187

fbshipit-source-id: e7970ed1c746a5bc28c15cfcf5950ee7bc79d67c

proxygen/lib/http/sink/HTTPTunnelSink.cpp

@@ -32,9 +32,13 @@ void HTTPTunnelSink::detachAndAbortIfIncomplete(
 }
 
 void HTTPTunnelSink::sendBody(std::unique_ptr<folly::IOBuf> body) {
+  DestructorCheck::Safety safety(*this);
   resetIdleTimeout();
   ++outstandingWrites_;
   sock_->writeChain(this, std::move(body));
+  if (safety.destroyed()) {
+    return;
+  }
   if (outstandingWrites_ >= kMaxOutstandingWrites && !handlerEgressPaused_) {
     handlerEgressPaused_ = true;
     handler_->onEgressPaused();
@@ -91,9 +95,12 @@ void HTTPTunnelSink::timeoutExpired() noexcept {
   XLOG(DBG4) << "Closing socket now";
   sock_->closeNow();
   if (handler_) {
+    DestructorCheck::Safety safety(*this);
     handler_->onError(HTTPException(
         HTTPException::Direction::INGRESS_AND_EGRESS, "Idle timeout expired"));
-    handler_->detachTransaction();
+    if (!safety.destroyed() && handler_) {
+      handler_->detachTransaction();
+    }
   }
   idleTimeout_ = std::chrono::milliseconds(0);
 }
@@ -136,9 +143,12 @@ void HTTPTunnelSink::readEOF() noexcept {
 }
 
 void HTTPTunnelSink::readErr(const folly::AsyncSocketException& err) noexcept {
+  DestructorCheck::Safety safety(*this);
   handler_->onError(
       HTTPException(HTTPException::Direction::INGRESS_AND_EGRESS, err.what()));
-  handler_->detachTransaction();
+  if (!safety.destroyed() && handler_) {
+    handler_->detachTransaction();
+  }
 }
 
 // Returns true if this sink is destroyed
@@ -170,9 +180,12 @@ void HTTPTunnelSink::writeErr(size_t,
                               const folly::AsyncSocketException& err) noexcept {
   bool destroyed = writeComplete();
   if (!destroyed && handler_) {
+    DestructorCheck::Safety safety(*this);
     handler_->onError(HTTPException(
         HTTPException::Direction::INGRESS_AND_EGRESS, err.what()));
-    handler_->detachTransaction();
+    if (!safety.destroyed() && handler_) {
+      handler_->detachTransaction();
+    }
   }
 }
 

proxygen/lib/http/sink/test/HTTPTunnelSinkTest.cpp

@@ -168,8 +168,8 @@ TEST_F(HTTPTunnelSinkTest, test_idle_timeout) {
   // Wait another 8ms; the timeout was reset, so this still shouldn't expire
   sleep_evb(evb_, std::chrono::milliseconds(8));
 
-  // Ok, now, expect a call to detachTransaction and closeNow which happens
-  // during timeout expiration
+  // Ok, now, expect callbacks which happen during timeout expiration
+  EXPECT_CALL(*mockHandler_, onError).WillOnce(Return());
   EXPECT_CALL(*mockHandler_, detachTransaction).WillOnce(Return());
   EXPECT_CALL(*mockSocket_, closeNow).WillOnce(Return());
   sleep_evb(evb_, std::chrono::milliseconds(8));
@@ -228,8 +228,17 @@ TEST_F(HTTPTunnelSinkTest, test_send_abort) {
 }
 
 TEST_F(HTTPTunnelSinkTest, test_read_error) {
-  EXPECT_CALL(*mockHandler_, detachTransaction).WillOnce(Return());
   EXPECT_CALL(*mockHandler_, onError).WillOnce(Return());
+  EXPECT_CALL(*mockHandler_, detachTransaction).WillOnce(Return());
+  sink_->readErr(folly::AsyncSocketException(
+      folly::AsyncSocketException::AsyncSocketExceptionType::UNKNOWN, "test"));
+}
+
+TEST_F(HTTPTunnelSinkTest, test_read_error_with_abort) {
+  EXPECT_CALL(*mockHandler_, onError).WillOnce(Invoke([&](auto &&) {
+    sink_->detachAndAbortIfIncomplete(std::move(sink_));
+  }));
+  EXPECT_CALL(*mockHandler_, detachTransaction).Times(0);
   sink_->readErr(folly::AsyncSocketException(
       folly::AsyncSocketException::AsyncSocketExceptionType::UNKNOWN, "test"));
 }
@@ -248,4 +257,20 @@ TEST_F(HTTPTunnelSinkTest, test_write_error) {
   sink_->sendBody(folly::IOBuf::copyBuffer("hello world"));
 }
 
+TEST_F(HTTPTunnelSinkTest, test_write_error_with_abort) {
+  EXPECT_CALL(*mockSocket_, writeChain)
+      .WillRepeatedly(Invoke([](WriteCB *cb, auto &&, auto &&) {
+        cb->writeErr(
+            0,
+            folly::AsyncSocketException(
+                folly::AsyncSocketException::AsyncSocketExceptionType::UNKNOWN,
+                "test"));
+      }));
+  EXPECT_CALL(*mockHandler_, onError).WillOnce(Invoke([&](auto &&) {
+    sink_->detachAndAbortIfIncomplete(std::move(sink_));
+  }));
+  EXPECT_CALL(*mockHandler_, detachTransaction).Times(0);
+  sink_->sendBody(folly::IOBuf::copyBuffer("hello world"));
+}
+
 } // namespace proxygen