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

Conversation

Projects
None yet
4 participants
@ghedo
Member

ghedo commented Dec 10, 2016

No description provided.

@mention-bot

This comment has been minimized.

mention-bot commented Dec 10, 2016

@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

This comment has been minimized.

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

This comment has been minimized.

Member

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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

Member

ghedo commented Jan 14, 2017

@jay mind reviewing again?

@ghedo

This comment has been minimized.

Member

ghedo commented Jan 14, 2017

Also fixed the failing test now.

@jay

jay approved these changes Jan 14, 2017

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

2 of 3 checks passed

continuous-integration/travis-ci/push The Travis CI build is in progress
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

peterpih pushed a commit to railsnewbie257/curl that referenced this pull request Jan 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment