follow-up for D69196385

Summary: contents of this diff weren't absorbed into D69196385 prior to landing, publishing separately

Reviewed By: HsiehYuho

Differential Revision: D71130919

fbshipit-source-id: ac947681ef6f0def01f0ca58aa6bda770b0a879d

Author: hanidamlaj

proxygen/lib/http/session/HTTPTransaction.cpp

@@ -650,7 +650,7 @@ void HTTPTransaction::invariantViolation(HTTPException ex) {
   }
   // In http/1.1, this will send TCP reset and ungracefully terminate the
   // connection. In h2, this will send stream reset but keep the connection
-  // open.
+  // open. Should this be INTERNAL_ERROR?
   sendAbort(ErrorCode::NO_ERROR);
 }
 

proxygen/lib/http/session/HTTPTransaction.h

@@ -257,7 +257,6 @@ class HTTPTransactionHandler : public TraceEventObserver {
    * invoked in cases that violate an internal invariant that is fatal to the
    * transaction but can be recoverable for the session or library.  One such
    * example is mis-use of the egress APIs (sendBody() before sendHeaders()).
-   *
    */
   virtual void onInvariantViolation(const HTTPException& error) noexcept {
     LOG(FATAL) << error.what();

proxygen/lib/http/session/test/DownstreamTransactionTest.cpp

@@ -158,6 +158,7 @@ TEST_F(DownstreamTransactionTest, InvariantViolationHandler) {
   EXPECT_CALL(transport_, sendAbort(_, _));
   EXPECT_CALL(handler_, _detachTransaction());
   EXPECT_CALL(transport_, detach(&txn));
+
   txn.setHandler(&handler_);
   // Send body before headers -- oops;
   txn.sendBody(makeBuf(10));

proxygen/lib/http/session/test/HTTPDownstreamSessionTest.cpp

@@ -4947,3 +4947,61 @@ TEST_F(HTTP2DownstreamSessionTest, Observer_PreWrite) {
 
   expectDetachSession();
 }
+
+TEST_F(HTTPDownstreamSessionTest, InvariantViolation) {
+  folly::DelayedDestruction::DestructorGuard g(httpSession_);
+
+  auto handler = addSimpleStrictHandler();
+  auto& txn = handler->txn_;
+  // attempting to egress body first => invariantViolation
+  handler->expectHeaders([&]() { handler->sendBody(100); });
+
+  EXPECT_CALL(*handler, _onInvariantViolation(_)).WillOnce([&]() {
+    // invariantViolation callback can send headers if allowed
+    CHECK(txn->canSendHeaders());
+    txn->sendHeaders(getResponse(500, 0));
+  });
+
+  sendRequest(getGetRequest(), /*eom=*/true);
+
+  // this should detach transaction since invariantViolation triggers abort
+  EXPECT_CALL(*handler, _detachTransaction);
+  flushRequestsAndLoopN(1);
+  evbLoopNonBlockN(1);
+
+  EXPECT_CALL(callbacks_, onHeadersComplete(_, _)).WillOnce([](auto, auto msg) {
+    EXPECT_EQ(msg->getStatusCode(), 500);
+  });
+
+  parseOutput(*clientCodec_);
+  gracefulShutdown();
+}
+
+TEST_F(HTTP2DownstreamSessionTest, InvariantViolation) {
+  auto handler = addSimpleStrictHandler();
+  auto& txn = handler->txn_;
+  // attempting to egress body first => invariantViolation
+  handler->expectHeaders([&]() { handler->sendBody(100); });
+
+  EXPECT_CALL(*handler, _onInvariantViolation(_)).WillOnce([&]() {
+    // invariantViolation callback can send headers if allowed
+    CHECK(txn->canSendHeaders());
+    txn->sendHeaders(getResponse(500, 0));
+    txn->sendEOM();
+  });
+
+  sendRequest(getGetRequest(), /*eom=*/true);
+
+  // this should detach transaction since invariantViolation triggers abort
+  EXPECT_CALL(*handler, _detachTransaction);
+  flushRequestsAndLoopN(1);
+  evbLoopNonBlockN(2);
+
+  EXPECT_CALL(callbacks_, onHeadersComplete(_, _)).WillOnce([](auto, auto msg) {
+    EXPECT_EQ(msg->getStatusCode(), 500);
+  });
+  EXPECT_CALL(callbacks_, onAbort(_, ErrorCode::NO_ERROR));
+
+  parseOutput(*clientCodec_);
+  gracefulShutdown();
+}