Fix frame generation order

Summary: HTTP/2 requires that clients and servers send their connection preface/settings respectively as their first frame on the session. Move generate* calls into the appropriate location in the constructor and startNow. These were moved out in an ancient diff (D836488) that was trying to move async action out of the ctor, but generate* should just put bytes into writeBuf_. Note I think it is possible for upstream clients that call send* from setTransaction to insert frames before SETTINGS, which may cause problems. Test Plan: Unit tests, canary HTTP/2 Reviewed By: [email protected] Subscribers: doug, bmatheny FB internal diff: D1932061 Tasks: 6543821 Signature: t1:1932061:1426891893:8e66dd0504283dab0b3088ba39a69a8691fac534
mzhaom · Mar 25, 2015 · f53c316 · f53c316
1 parent 2cab20c
commit f53c316
Show file tree

Hide file tree

Showing 5 changed files with 21 additions and 11 deletions.
diff --git a/proxygen/lib/http/codec/FlowControlFilter.cpp b/proxygen/lib/http/codec/FlowControlFilter.cpp
@@ -22,6 +22,8 @@ HTTPException getException(const std::string& msg) {
 
 }
 
+const uint32_t FlowControlFilter::kDefaultCapacity = spdy::kInitialWindow;
+
 FlowControlFilter::FlowControlFilter(Callback& callback,
                                      folly::IOBufQueue& writeBuf,
                                      HTTPCodec* codec,

diff --git a/proxygen/lib/http/codec/FlowControlFilter.h b/proxygen/lib/http/codec/FlowControlFilter.h
@@ -36,6 +36,8 @@ class FlowControlFilter:
     virtual void onConnectionSendWindowClosed() = 0;
   };
 
+  static const uint32_t kDefaultCapacity;
+
   /**
    * Construct a flow control filter.
    * @param callback     A channel to be notified when the window is not
@@ -44,13 +46,15 @@ class FlowControlFilter:
    *                     may generate a window update frame on this buffer.
    * @param codec        The codec implementation.
    * @param recvCapacity The initial size of the conn-level recv window.
-   *                     It must be >= 65536.
+   *                     It must be >= 65536.  If greater than 65536 it
+   *                     will generate an immediate window update into
+   *                     writeBuf.
    */
   explicit
   FlowControlFilter(Callback& callback,
                     folly::IOBufQueue& writeBuf,
                     HTTPCodec* codec,
-                    uint32_t recvCapacity);
+                    uint32_t recvCapacity = kDefaultCapacity);
 
   /**
    * Modify the session receive window

diff --git a/proxygen/lib/http/session/HTTPSession.cpp b/proxygen/lib/http/session/HTTPSession.cpp
@@ -146,11 +146,11 @@ HTTPSession::HTTPSession(
     settings->setSetting(SettingsId::MAX_CONCURRENT_STREAMS,
                          maxConcurrentIncomingStreams_);
   }
+
+  codec_->generateConnectionPreface(writeBuf_);
+
   if (codec_->supportsSessionFlowControl()) {
-    connFlowControl_ = new FlowControlFilter(*this,
-                                             writeBuf_,
-                                             codec_.call(),
-                                             kDefaultReadBufLimit);
+    connFlowControl_ = new FlowControlFilter(*this, writeBuf_, codec_.call());
     codec_.addFilters(std::unique_ptr<FlowControlFilter>(connFlowControl_));
   }
 
@@ -191,8 +191,11 @@ HTTPSession::~HTTPSession() {
 void HTTPSession::startNow() {
   CHECK(!started_);
   started_ = true;
-  codec_->generateConnectionPreface(writeBuf_);
   codec_->generateSettings(writeBuf_);
+  if (connFlowControl_) {
+    connFlowControl_->setReceiveWindowSize(writeBuf_,
+                                           receiveSessionWindowSize_);
+  }
   scheduleWrite();
   resumeReads();
 }
@@ -214,15 +217,13 @@ void HTTPSession::setFlowControl(size_t initialReceiveWindow,
   CHECK(!started_);
   initialReceiveWindow_ = initialReceiveWindow;
   receiveStreamWindowSize_ = receiveStreamWindowSize;
+  // TODO(t6558522): line up kDefaultReadBufLimit and receiveSessionWindowSize
+  receiveSessionWindowSize_ = receiveSessionWindowSize;
   HTTPSettings* settings = codec_->getEgressSettings();
   if (settings) {
     settings->setSetting(SettingsId::INITIAL_WINDOW_SIZE,
                          initialReceiveWindow_);
   }
-  if (connFlowControl_) {
-    connFlowControl_->setReceiveWindowSize(writeBuf_, receiveSessionWindowSize);
-    scheduleWrite();
-  }
 }
 
 void HTTPSession::setMaxConcurrentOutgoingStreams(uint32_t num) {

diff --git a/proxygen/lib/http/session/HTTPSession.h b/proxygen/lib/http/session/HTTPSession.h
@@ -847,6 +847,7 @@ class HTTPSession:
   // Flow control settings
   size_t initialReceiveWindow_{65536};
   size_t receiveStreamWindowSize_{65536};
+  size_t receiveSessionWindowSize_{65536};
 
   const TransportDirection direction_;
 

diff --git a/proxygen/lib/http/session/test/HTTPDownstreamSessionTest.cpp b/proxygen/lib/http/session/test/HTTPDownstreamSessionTest.cpp
@@ -1725,6 +1725,8 @@ TEST_F(SPDY31DownstreamTest, testSessionFlowControl) {
   SPDYCodec clientCodec(TransportDirection::UPSTREAM,
                         SPDYVersion::SPDY3_1);
 
+  InSequence sequence;
+  EXPECT_CALL(callbacks, onSettings(_));
   EXPECT_CALL(callbacks, onWindowUpdate(0, spdy::kInitialWindow));
   clientCodec.setCallback(&callbacks);
   parseOutput(clientCodec);