-
Notifications
You must be signed in to change notification settings - Fork 127
Open
Description
I overlooked the fact that the http2 settings will technically be ordered with InitialWindowSize and HeaderTableSize always being the last two header frames sent. No site is detecting via http2 setting order at the moment, so it is not problem, but the fix is also pretty easy. In order to fix this error, there needs to be an underlying API change detailed below. What are everyone's opinions on this?
Code that creates error:
setMaxHeader := false
if t.Settings != nil {
for _, setting := range t.Settings {
if setting.ID == SettingMaxHeaderListSize {
setMaxHeader = true
}
if setting.ID == SettingHeaderTableSize || setting.ID == SettingInitialWindowSize {
return nil, errSettingsIncludeIllegalSettings
}
initialSettings = append(initialSettings, setting)
}
}
if t.InitialWindowSize != 0 {
initialSettings = append(initialSettings, Setting{ID: SettingInitialWindowSize, Val: t.InitialWindowSize})
} else {
initialSettings = append(initialSettings, Setting{ID: SettingInitialWindowSize, Val: transportDefaultStreamFlow})
}
if t.HeaderTableSize != 0 {
initialSettings = append(initialSettings, Setting{ID: SettingHeaderTableSize, Val: t.HeaderTableSize})
} else {
initialSettings = append(initialSettings, Setting{ID: SettingHeaderTableSize, Val: initialHeaderTableSize})
}
if max := t.maxHeaderListSize(); max != 0 && !setMaxHeader {
initialSettings = append(initialSettings, Setting{ID: SettingMaxHeaderListSize, Val: max})
}Proposed Fix:
- Have the http2 Transport Settings field changed from
// Settings should not include InitialWindowSize or HeaderTableSize, set that in Transport
Settings []Setting
InitialWindowSize uint32 // if nil, will use global initialWindowSize
HeaderTableSize uint32 // if nil, will use global initialHeaderTableSizeto
Settings map[SettingID]uint32This would require everyone using the library to change their code, but since this isn't a pressing issue that seems like unnecessary work.
Metadata
Metadata
Assignees
Labels
No labels