Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ignore flow control in h2 when sending first request #849

Merged
merged 6 commits into from
Jul 22, 2019

Conversation

zyearn
Copy link
Member

@zyearn zyearn commented Jul 19, 2019

No description provided.

if (!ParseH2Settings(&_remote_settings, it, frame_head.payload_size)) {
LOG(ERROR) << "Fail to parse from SETTINGS";
return MakeH2Error(H2_PROTOCOL_ERROR);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

首先这段代码可以如下简化:

if (!_remote_settings_received) {        
        _remote_settings.stream_window_size = H2Settings::DEFAULT_INITIAL_WINDOW_SIZE;
        _remote_window_left.fetch_sub(
                H2Settings::MAX_WINDOW_SIZE - H2Settings::DEFAULT_INITIAL_WINDOW_SIZE,
                butil::memory_order_relaxed);
        _remote_settings_received = true;
}
if (!ParseH2Settings(&_remote_settings, it, frame_head.payload_size)) {
        LOG(ERROR) << "Fail to parse from SETTINGS";
        return MakeH2Error(H2_PROTOCOL_ERROR);
}

其次目前的改法在WINDOWS_UPDATE过来前,还是有段时间_remote_window_left已经归位了,理论上做不到“只要server端connection-level window size设得很大就总能发送成功”,不过窗口比较小

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1.这样简化是不是有个race:当client端第一个setting包收到后,将stream_window_size和_remote_window_left复原了,此时一个req的AppendAndDestroy正在调用,发现window太小,就发失败了。现在的pr代码可以避免”复原到初始“这个中间态。

2.从server的视角来说,client的default connection window size一定是65536,所以一定是server端来负责在合适的时候发WINDOWS_UPDATE来保证client可以一直发。如果接收到setting后,并且WINDOWS_UPDATE一直没发过来,这时候发送失败是个预期的行为。如果server的connection size很大,正确的实现是发完setting后立刻发WINDOWS_UPDATE(让client立刻感知),现在brpc server的代码就是这么实现的

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants