Navigation Menu

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

Disable h2 server push if not requested #1160

Merged
merged 1 commit into from Jan 15, 2017
Merged

Disable h2 server push if not requested #1160

merged 1 commit into from Jan 15, 2017

Conversation

ghedo
Copy link
Contributor

@ghedo ghedo commented Dec 10, 2016

No description provided.

@mention-bot
Copy link

@ghedo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bagder, @tatsuhiro-t and @Andersbakken to be potential reviewers.

@jay
Copy link
Member

jay commented Dec 10, 2016

I'm unclear on whether the server can just push to us without acceptance, however if this is the case now that we have large windows by default this change might be helpful. There is aproblem with the way you implemented it because we have a global settings we don't expect to be modified, so say for example you change it in one multi and some other one subsequent or another thread needs it off.

Also settings[2].value = 1 doesn't read well and the index could change if it's modified later. I'd rather check for it or put it in a function like

if(data->multi->push_cb) {
  int i;
  for(i = 0; i < sizeof(settings)/sizeof(settings[0]); ++i) {
    if(settings[i].settings_id == NGHTTP2_SETTINGS_ENABLE_PUSH) {
      settings[i].value = 1;
    }
  }
}
or even easier to understand:
if(!data->multi->push_cb)
  add_setting(settings, NGHTTP2_SETTINGS_ENABLE_PUSH, 0);
then in add_setting check settings for that setting and if not found add it as 0

So I am not one of the http2 experts here but I think this would involve renaming settings to default settings and then giving each conn a copy of the default settings which it could then modify, then modifying those settings to disable push. or maybe just a make_settings() that contains all the defaults and modifies them based on whatever

edit: also test1800 and test1801 will need to have the HTTP2-Settings line modified for the new default settings

@jay jay added the HTTP/2 label Dec 10, 2016
@ghedo
Copy link
Contributor Author

ghedo commented Dec 10, 2016

I'm unclear on whether the server can just push to us without acceptance

Yes, the server can send any data until the client resets the stream, which libcurl does if there is no push_cb. However by that time the server might have sent some data already (which would be a waste IMO).

we have a global settings we don't expect to be modified

Oh, right, I'm dumb.

The easier implementation I think would be to keep an empty array on the stack and populate it manually like nghttp does https://github.com/nghttp2/nghttp2/blob/master/src/nghttp.cc#L846

Which would also make it easier to add more user-defined settings in the future, if that ever is needed.

@jay
Copy link
Member

jay commented Dec 10, 2016

well you could put the settings array in http_conn because more than one function needs access to it (Curl_http2_switched, Curl_http2_request_upgrade) and populate it similarly like populate_settings(httpc->settings)

@bagder
Copy link
Member

bagder commented Jan 13, 2017

Will you do a follow-up to this or should we close it until you or someone else takes another shot at it?

@ghedo
Copy link
Contributor Author

ghedo commented Jan 14, 2017

@jay mind reviewing again?

@ghedo
Copy link
Contributor Author

ghedo commented Jan 14, 2017

Also fixed the failing test now.

Copy link
Member

@jay jay left a comment

Choose a reason for hiding this comment

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

LGTM. Though test1801 is disabled I would update it with the changed HTTP2-Settings.

@ghedo ghedo merged commit 9ad034e into curl:master Jan 15, 2017
peterpih pushed a commit to railsnewbie257/curl that referenced this pull request Jan 24, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

4 participants