untested support for goaway & pass stream_id via HttpResponseSender instead of Controller

src/brpc/controller.cpp

@@ -362,19 +362,15 @@ void Controller::AppendServerIdentiy() {
     }
 }
 
-// Defined in http_rpc_protocol.cpp
-namespace policy {
-int ErrorCode2StatusCode(int error_code);
-}
-
 inline void UpdateResponseHeader(Controller* cntl) {
     DCHECK(cntl->Failed());
-    if (cntl->request_protocol() == PROTOCOL_HTTP) {
+    if (cntl->request_protocol() == PROTOCOL_HTTP ||
+        cntl->request_protocol() == PROTOCOL_HTTP2) {
         if (cntl->ErrorCode() != EHTTP) {
-            // We assume that status code is already set along with EHTTP.
+            // Set the related status code
             cntl->http_response().set_status_code(
-                policy::ErrorCode2StatusCode(cntl->ErrorCode()));
-        }
+                ErrorCodeToStatusCode(cntl->ErrorCode()));
+        } // else assume that status code is already set along with EHTTP.
         if (cntl->server() != NULL) {
             // Override HTTP body at server-side to conduct error text
             // to the client.

src/brpc/http2.cpp

@@ -12,10 +12,11 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-#include "brpc/http2.h"
-#include "brpc/details/hpack.h"
 #include <limits>
 #include "butil/logging.h"
+#include "brpc/details/hpack.h"
+#include "brpc/errno.pb.h"
+#include "brpc/http2.h"
 
 namespace brpc {
 

src/brpc/http_header.cpp

@@ -24,9 +24,7 @@ namespace brpc {
 HttpHeader::HttpHeader() 
     : _status_code(HTTP_STATUS_OK)
     , _method(HTTP_METHOD_GET)
-    , _version(1, 1)
-    , _h2_stream_id(0)
-    , _h2_error(H2_NO_ERROR) {
+    , _version(1, 1) {
     // NOTE: don't forget to clear the field in Clear() as well.
 }
 
@@ -50,8 +48,6 @@ void HttpHeader::Swap(HttpHeader &rhs) {
     _content_type.swap(rhs._content_type);
     _unresolved_path.swap(rhs._unresolved_path);
     std::swap(_version, rhs._version);
-    std::swap(_h2_stream_id, rhs._h2_stream_id);
-    std::swap(_h2_error, rhs._h2_error);
 }
 
 void HttpHeader::Clear() {
@@ -62,8 +58,6 @@ void HttpHeader::Clear() {
     _content_type.clear();
     _unresolved_path.clear();
     _version = std::make_pair(1, 1);
-    _h2_stream_id = 0;
-    _h2_error = H2_NO_ERROR;
 }
 
 const char* HttpHeader::reason_phrase() const {

src/brpc/http_header.h

@@ -63,12 +63,6 @@ public:
     // True if the message is from HTTP2.
     bool is_http2() const { return major_version() == 2; }
 
-    // Id of the HTTP2 stream where the message is from.
-    // 0 when is_http2() is false.
-    int h2_stream_id() const { return _h2_stream_id; }
-
-    H2Error h2_error() const { return _h2_error; }
-
     // Get/set "Content-Type". Notice that you can't get "Content-Type"
     // via GetHeader().
     // possible values: "text/plain", "application/json" ...
@@ -160,8 +154,6 @@ friend void policy::ProcessHttpRequest(InputMessageBase *msg);
     std::string _content_type;
     std::string _unresolved_path;
     std::pair<int, int> _version;
-    int _h2_stream_id;
-    H2Error _h2_error;
 };
 
 const HttpHeader& DefaultHttpHeader();

src/brpc/http_status_code.cpp

@@ -19,7 +19,7 @@
 #include "butil/logging.h"                           // BAIDU_*
 #include "butil/macros.h"                            // ARRAY_SIZE
 #include "butil/thread_local.h"                      // thread_local
-
+#include "brpc/errno.pb.h"
 #include "brpc/http_status_code.h"
 
 
@@ -113,4 +113,30 @@ const char *HttpReasonPhrase(int status_code) {
     return tls_phrase_cache;
 }
 
+int ErrorCodeToStatusCode(int error_code) {
+    if (error_code == 0) {
+        return HTTP_STATUS_OK;
+    }
+    switch (error_code) {
+    case ENOSERVICE:
+    case ENOMETHOD:
+        return HTTP_STATUS_NOT_FOUND;
+    case ERPCAUTH:
+        return HTTP_STATUS_UNAUTHORIZED;
+    case EREQUEST:
+    case EINVAL:
+        return HTTP_STATUS_BAD_REQUEST;
+    case ELIMIT:
+    case ELOGOFF:
+        return HTTP_STATUS_SERVICE_UNAVAILABLE;
+    case EPERM:
+        return HTTP_STATUS_FORBIDDEN;
+    case ERPCTIMEDOUT:
+    case ETIMEDOUT:
+        return HTTP_STATUS_GATEWAY_TIMEOUT;
+    default:
+        return HTTP_STATUS_INTERNAL_SERVER_ERROR;
+    }
+}
+
 } // namespace brpc

src/brpc/http_status_code.h

@@ -76,6 +76,9 @@ namespace brpc {
 // value into a container. Directly copy the memory instead.
 const char *HttpReasonPhrase(int status_code);
 
+// Convert brpc error code to related status code.
+int ErrorCodeToStatusCode(int error_code);
+
 // Informational 1xx
 // This class of status code indicates a provisional response, consisting
 // only of the Status-Line and optional headers, and is terminated by an

src/brpc/policy/http2_rpc_protocol.h

@@ -185,7 +185,7 @@ private:
 private:
     butil::atomic<int> _nref;
     uint32_t _size;
-    uint32_t _stream_id;
+    int _stream_id;
     mutable butil::Mutex _mutex;
     Controller* _cntl;
     std::unique_ptr<H2StreamContext> _sctx;
@@ -194,7 +194,7 @@ private:
 
 class H2UnsentResponse : public SocketMessage {
 public:
-    static H2UnsentResponse* New(Controller* c);
+    static H2UnsentResponse* New(Controller* c, int stream_id);
     void Destroy();
     void Describe(butil::IOBuf*) const;
     // @SocketMessage
@@ -208,9 +208,9 @@ private:
     void push(const std::string& name, const std::string& value)
     { new (&_list[_size++]) HPacker::Header(name, value); }
 
-    H2UnsentResponse(Controller* c)
+    H2UnsentResponse(Controller* c, int stream_id)
         : _size(0)
-        , _stream_id(c->http_request().h2_stream_id())
+        , _stream_id(stream_id)
         , _http_response(c->release_http_response()) {
         _data.swap(c->response_attachment());
     }
@@ -251,7 +251,7 @@ public:
     void set_correlation_id(uint64_t cid) { _correlation_id = cid; }
     
     size_t parsed_length() const { return this->_parsed_length; }
-    int stream_id() const { return header().h2_stream_id(); }
+    int stream_id() const { return _stream_id; }
 
     int64_t ReleaseDeferredWindowUpdate() {
         if (_deferred_window_update.load(butil::memory_order_relaxed) == 0) {
@@ -272,6 +272,7 @@ friend class H2Context;
 #if defined(BRPC_H2_STREAM_STATE)
     H2StreamState _state;
 #endif
+    int _stream_id;
     bool _stream_ended;
     butil::atomic<int64_t> _remote_window_left;
     butil::atomic<int64_t> _deferred_window_update;
@@ -336,9 +337,9 @@ public:
     int AllocateClientStreamId();
     bool RunOutStreams() const;
     // Try to map stream_id to ctx if stream_id does not exist before
-    // Returns true on success, false otherwise.
-    bool TryToInsertStream(int stream_id, H2StreamContext* ctx);
-    uint32_t VolatilePendingStreamSize() const;
+    // Returns 0 on success, -1 on exist, 1 on goaway.
+    int TryToInsertStream(int stream_id, H2StreamContext* ctx);
+    size_t VolatilePendingStreamSize() const { return _pending_streams.size(); }
 
     HPacker& hpacker() { return _hpacker; }
     const H2Settings& remote_settings() const { return _remote_settings; }
@@ -372,6 +373,8 @@ friend void InitFrameHandlers();
     H2ParseResult OnContinuation(butil::IOBufBytesIterator&, const H2FrameHead&);
 
     H2StreamContext* RemoveStream(int stream_id);
+    void RemoveGoAwayStreams(int goaway_stream_id, std::vector<H2StreamContext*>* out_streams);
+
     H2StreamContext* FindStream(int stream_id);
     void ClearAbandonedStreamsImpl();
 
@@ -382,6 +385,7 @@ friend void InitFrameHandlers();
     H2ConnectionState _conn_state;
     int _last_server_stream_id;
     uint32_t _last_client_stream_id;
+    int _goaway_stream_id;
     H2Settings _remote_settings;
     H2Settings _local_settings;
     H2Settings _unack_local_settings;
@@ -394,6 +398,19 @@ friend void InitFrameHandlers();
     butil::atomic<int64_t> _deferred_window_update;
 };
 
+inline int H2Context::AllocateClientStreamId() {
+    if (RunOutStreams()) {
+        return -1;
+    }
+    const int id = _last_client_stream_id;
+    _last_client_stream_id += 2;
+    return id;
+}
+
+inline bool H2Context::RunOutStreams() const {
+    return (_last_client_stream_id > 0x7FFFFFFF);
+}
+
 }  // namespace policy
 } // namespace brpc
 

src/brpc/socket.h

@@ -489,6 +489,8 @@ public:
     // Returns true if the remote side is overcrowded.
     bool is_overcrowded() const { return _overcrowded; }
 
+    bthread_keytable_pool_t* keytable_pool() const { return _keytable_pool; }
+
 private:
     DISALLOW_COPY_AND_ASSIGN(Socket);
 

test/brpc_http_rpc_protocol_unittest.cpp

@@ -211,13 +211,12 @@ protected:
         brpc::Controller cntl;
         test::EchoResponse res;
         res.set_message(EXP_RESPONSE);
-        cntl.http_request()._h2_stream_id = h2_stream_id;
         cntl.http_request().set_content_type("application/proto");
         {
             butil::IOBufAsZeroCopyOutputStream wrapper(&cntl.response_attachment());
             EXPECT_TRUE(res.SerializeToZeroCopyStream(&wrapper));
         }
-        brpc::policy::H2UnsentResponse* h2_res = brpc::policy::H2UnsentResponse::New(&cntl);
+        brpc::policy::H2UnsentResponse* h2_res = brpc::policy::H2UnsentResponse::New(&cntl, h2_stream_id);
         butil::Status st = h2_res->AppendAndDestroySelf(out, _h2_client_sock.get());
         ASSERT_TRUE(st.ok());
     }
@@ -687,7 +686,7 @@ TEST_F(HttpTest, read_long_body_progressively) {
         {
             brpc::Channel channel;
             brpc::ChannelOptions options;
-            options.protocol = brpc::PROTOCOL_HTTP2;
+            options.protocol = brpc::PROTOCOL_HTTP;
             ASSERT_EQ(0, channel.Init(butil::EndPoint(butil::my_ip(), port), &options));
             {
                 brpc::Controller cntl;
@@ -1109,7 +1108,7 @@ TEST_F(HttpTest, http2_window_used_up) {
         if (i == nsuc) {
             // the last message should fail according to flow control policy.
             ASSERT_FALSE(st.ok());
-            ASSERT_TRUE(st.error_code() == EAGAIN);
+            ASSERT_TRUE(st.error_code() == brpc::ELIMIT);
             ASSERT_TRUE(butil::StringPiece(st.error_str()).starts_with("remote_window_left is not enough"));
         } else {
             ASSERT_TRUE(st.ok());