stepancheg / grpc-rust

Rust implementation of gRPC
MIT License
1.37k stars 124 forks source link

Send an empty SETTINGS frame as server preface. #66

Closed disksing closed 7 years ago

disksing commented 7 years ago

As it is described in rfc(https://http2.github.io/http2-spec/#rfc.section.3.5):

The server connection preface consists of a potentially empty SETTINGS frame (Section 6.5) that MUST be the first frame the server sends in the HTTP/2 connection.

stepancheg commented 7 years ago

AFAIU, first SETTINGS frame sent by server is an ack SETTINGS frame to the first SETTINGS frame sent by client. And this is done by http server implementation:

pub fn server_handshake<I : Io + Send + 'static>(conn: I) -> HttpFuture<I> {
    let mut preface_buf = Vec::with_capacity(PREFACE.len());
    preface_buf.resize(PREFACE.len(), 0);
    let recv_preface = read_exact(conn, preface_buf)
        .map_err(|e| e.into())
        .and_then(|(conn, preface_buf)| {
            done(if preface_buf == PREFACE {
                Ok((conn))
            } else {
                Err(HttpError::InvalidFrame)
            })
        });

    let recv_settings = recv_preface.and_then(|conn| {
        recv_settings_frame(conn).map(|(conn, _)| conn)
    });

    let send_settings = recv_settings.and_then(|conn| {
        send_frame(conn, SettingsFrame::new_ack()) // <-- HERE
    });

    Box::new(send_settings)
}
disksing commented 7 years ago

@stepancheg I think there are some misunderstandings here. According to RFC, both endpoints should send a SETTINGS frame(without setting ACK) after the connection is established, and send a SETTINGS frame(with ACK) after receive SETTINGS from the other.

FYI, the latest release of netty checks the first frame should be SETTINGS and should not be ACK, and both send SETTINGS as the very first frame. Please refer to https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java#L319 https://github.com/netty/netty/blob/4.1/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java#L344

stepancheg commented 7 years ago

Seems like you are correct. Thanks for pointing out. I've fixed it differently in b4dae8e18ef19ef5b4da8f3df7f3bec503622c82. Thanks!

stepancheg commented 7 years ago

My fix was incorrect. Go server sends:

[FrameHeader SETTINGS len=0]
[FrameHeader WINDOW_UPDATE len=4]
  Window-Increment = 983025
[FrameHeader SETTINGS flags=ACK len=0]

i. e. it sends WINDOW_UPDATE before SETTINGS ACK.

stepancheg commented 7 years ago

Fixed now.