transport: fix race between applying and acking flow-control settings frames #1630
Conversation
MakMukhi
force-pushed
the
fix_settings_ack_race
branch
from
fd05b66
to
73d71a0
Compare
transport/http2_client.go
Outdated
| } | ||
| t.applySettings(i.rs) | ||
| t.controlBuf.put(&settingsAck{}) |
There was a problem hiding this comment.
Why not apply settings before enqueuing the ack the first time?
This results in more delay in processing the ack (it has to make it through the control buffer twice), and it splits up the code that handles settings, which makes it harder to reason about.
If there's some reason we have to do it this way, that's fine but we should put comments in to explain it.
There was a problem hiding this comment.
I was in fact thinking about that last night. The code sort of evolved to be this way and you're right it makes more sense to apply the settings in the settings handler itself.
transport/http2_client.go
Outdated
| t.mu.Unlock() | ||
| case http2.SettingInitialWindowSize: | ||
| t.mu.Lock() | ||
| if s.Val < t.streamSendQuota { |
There was a problem hiding this comment.
Shouldn't we be comparing this to t.initialWindowSize?
Also, do we have to take a lock for this field or t.maxStreams? I was thinking the only thing that should be changing these fields is the handling of a settings frame (so, this code here)...
There was a problem hiding this comment.
t.initialWindowSize controls our inbound window size while t.streamSendQuota controls our outbound window size. So, when we get a settings frame to update our initial window size( the outbound one) we update this variable.
You're right about t.maxStreams; updating it never required a lock. I copied the original code and made the same mistake!
MakMukhi
force-pushed
the
fix_settings_ack_race
branch
from
68889f2
to
8c58e9f
Compare
transport/http2_client.go
Outdated
| func (t *http2Client) isRestrictive(s http2.Setting) bool { | ||
| switch s.ID { | ||
| case http2.SettingMaxConcurrentStreams: | ||
| if int(s.Val) < t.maxStreams { |
There was a problem hiding this comment.
Optional (but recommended): return int(s.Val) < t.maxStreams
| rs = append(rs, s) | ||
| } else { | ||
| ps = append(ps, s) | ||
| } | ||
| return nil | ||
| }) | ||
| if isFirst && isMaxConcurrentStreamsMissing { |
There was a problem hiding this comment.
Optional, maybe for a later refactor:
The code might be cleaner with two implementations: handleSettings and handleInitialSettings. handleInitialSettings doesn't need to check restrictive/permissive -- just apply all the settings and write the ack. Then the special-case code checking for max concurrent streams is removed from the ongoing handleSettings function.
There was a problem hiding this comment.
The same logic, acking after applying restrictive settings and before applying permissive settings, holds for handleInitialSettings. The client doesn't wait for the server to send initial settings before starting streams.
We'll still need to check if http2.SettingMaxConcurrentStreams is present or not in the initial settings. So we will only be able to save one conditional check by creating this other function.
dfawley
changed the title
fixes #1626 #1564