Skip to content
This repository was archived by the owner on Jul 6, 2018. It is now read-only.

More updates#150

Closed
jasnell wants to merge 1 commit intonodejs:masterfrom
jasnell:more-updates
Closed

More updates#150
jasnell wants to merge 1 commit intonodejs:masterfrom
jasnell:more-updates

Conversation

@jasnell
Copy link
Copy Markdown
Member

@jasnell jasnell commented Jun 7, 2017 •
edited
Loading

More updates... largely fixing and improving settings handling.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

mcollina

This comment was marked as off-topic.

Sign in to view

sebdeckers

This comment was marked as off-topic.

Sign in to view

@jasnell
Copy link
Copy Markdown
Member Author

jasnell commented Jun 7, 2017 •
edited
Loading

regarding settings() taking a callback, I was stewing over that also. It would be fairly trivial to support. Keep in mind, tho, that there is an initial SETTINGS frame that is sent immediately on connection set up that would get an ack and we really don't have a great way of setting a callback for that one (particularly on the server side). If we added an optional callback, should we also still emit 'localSettings'? (Note that registering the optional callback as a listener for 'localSettings' won't work since event emitter doesn't have a FIFO capability). @mcollina ... thoughts on this?

@jasnell
Copy link
Copy Markdown
Member Author

jasnell commented Jun 7, 2017

Added a bunch more to this PR, including improved options handling and a few more needed tests

mcollina

This comment was marked as off-topic.

Sign in to view

@jasnell
http2: multiple updates
587a8f4 
* unref the uv_prep_t handle
* remove unused code
* improve settings handling: add `'localSettings'` and
  `'remoteSettings'` events, improve handling of settings
  acknowledgements
* improve options handling
* ensure that max concurrent streams works correctly
* update documentation
* add return values in docs
* throw on pushStream if push not enabled

PR-URL: nodejs#150
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@jasnell
Copy link
Copy Markdown
Member Author

jasnell commented Jun 9, 2017

nit fixed, landed

@jasnell jasnell closed this Jun 9, 2017
jasnell added a commit that referenced this pull request Jun 9, 2017
@jasnell
http2: multiple updates
be3f534 
* unref the uv_prep_t handle
* remove unused code
* improve settings handling: add `'localSettings'` and
  `'remoteSettings'` events, improve handling of settings
  acknowledgements
* improve options handling
* ensure that max concurrent streams works correctly
* update documentation
* add return values in docs
* throw on pushStream if push not enabled

PR-URL: #150
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
jasnell added a commit to jasnell/http2-1 that referenced this pull request Jun 22, 2017
@jasnell
http2: multiple updates
3e3ac29 
* unref the uv_prep_t handle
* remove unused code
* improve settings handling: add `'localSettings'` and
  `'remoteSettings'` events, improve handling of settings
  acknowledgements
* improve options handling
* ensure that max concurrent streams works correctly
* update documentation
* add return values in docs
* throw on pushStream if push not enabled

PR-URL: nodejs#150
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
jasnell added a commit to jasnell/http2-1 that referenced this pull request Jul 10, 2017
@jasnell
http2: multiple updates
73e9be6 
* unref the uv_prep_t handle
* remove unused code
* improve settings handling: add `'localSettings'` and
  `'remoteSettings'` events, improve handling of settings
  acknowledgements
* improve options handling
* ensure that max concurrent streams works correctly
* update documentation
* add return values in docs
* throw on pushStream if push not enabled

PR-URL: nodejs#150
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
jasnell added a commit to jasnell/http2-1 that referenced this pull request Jul 14, 2017
@jasnell
http2: multiple updates
07675aa 
* unref the uv_prep_t handle
* remove unused code
* improve settings handling: add `'localSettings'` and
  `'remoteSettings'` events, improve handling of settings
  acknowledgements
* improve options handling
* ensure that max concurrent streams works correctly
* update documentation
* add return values in docs
* throw on pushStream if push not enabled

PR-URL: nodejs#150
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@mcollina mcollina mcollina approved these changes

@sebdeckers sebdeckers sebdeckers left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jasnell @mcollina @sebdeckers