facebook/proxygen

HTTP2 Protocol Error

SteveSelva opened this issue · 3 comments

I have modified the sample "proxygen_proxy" to work as a proxy server.
There I came up with a issue in HTTP2 Protocol.
I have configured the proxy server to limit maxConcurrentIncomingStreams = 100.
And a connection is established through proxy server, both Downstream and Upstream uses HTTP2 Protocol for communication.
Upstream sets MAX_CONCURRENT_STREAMS = 100 using SETTINGS Frame.
When the transaction id of the session = 201(upstream stream id = downstream stream id), the Upstream server returns PROTOCOL_ERROR as response and aborts the stream and all the other streams get closed with StreamUnacknowledged error.
But when I see the logs from HTTPSession::InfoCallback, I can see that logs from function: onTransactionDetached() on that Upstream Session. So I think the streams are closed too.
When I print the number of active streams, number of incoming streams and number of outgoing streams on onServerError() function in ProxyHandler, their values is also less than 100.
And I have confirmed multiple types the StreamAborted message in onServerError() function when the StreamID is 201.

Why is it happening?
Is the Streams are not properly closed in Upstream causing MAX_CONCURRENT_STREAMS > 100 to throw PROTOCOL_ERROR?

Can you provide a solution to fix this issue.

Found the cause of the issue, and it was that the Proxygen is sending END_STREAM Flag separately in a empty DATA Frame, instead it can send it with the HEADERS Frame itself. Modified it to send END_STREAM Flag within the HEADERS Frame and the issue got resolved.

Can you share a snippet of your code fix?

There's definitely some friction in proxies because the onHeadersComplete codec callback doesn't include the END_STREAM flag, so it can be challenging to immediately pass through to an upstream without making an assumption (eg: GET == END_STREAM) or delaying an event loop so the onEOM downstream callback fires.

I've even tried fixing this once, but ran into some complications.

I don't know whether this approach is right or not. But somehow it works for now.

HTTP2Codec.cpp
Line No:520

if (!(curHeader_.flags & http2::END_STREAM)) {
  // If it there are DATA frames coming, consider it chunked
  msg->setIsChunked(true);
}

This code tells that if the stream ends or not. If it doesn't end the HTTPMessage is set to chunked.
So in ProxyHandler, I am checking whether the CodecProtocol is HTTP2 and HTTPMessage is not chunked, if both conditions satisfy, then sending the Upstream transaction with EOM using sendHeadersWithEOM().

ProxyHandler.cpp

if (txn_->getTransport().getHTTPSessionBase()->getCodecProtocol() == proxygen::CodecProtocol::HTTP_2 && 
downstream_->getTransaction()->getTransport().getHTTPSessionBase()->getCodecProtocol() == proxygen::CodecProtocol::HTTP_2 &&
 !request_->getIsChunked()) {
    txn_->sendHeadersWithEOM(*request_);
}
else {
    txn_->sendHeaders(*request_);
}

This code checks whether both Upstream and Downstream works in HTTP2 and then checks whether the message is chunked which in turn is true if END_STREAM is not set.

Please provide some insights on whether this approach is right or not.