Implement server push #40

alekstorm · 2014-04-07T03:57:48Z

~~Depends on #39~~

See the changes to docs/source/advanced.rst for an overview.

Apologies for not getting approval for this ahead of time; you did clearly state in the documentation that server push wasn't a planned feature. However, I find it one of the most compelling parts of HTTP/2.0, and I think this a workable API for a single-threaded library. What do you think?

P.S. I also snuck in a fix for a bug that caused hyper to crash when trying to send a WINDOW_UPDATE frame to a server that had gone away with the stream half-closed on the remote side.

Lukasa · 2014-04-07T06:25:22Z

docs/source/advanced.rst

+
+``hyper`` does not currently provide any way to limit the number of pushed
+streams, disable them altogether, or cancel in-progress pushed streams, although
+HTTP/2.0 allows all of these actions.


Before we add this functionality we must provide an API for disabling SERVER_PUSH, if only for the sake of requests which does not expect them.

Lukasa · 2014-04-07T06:28:50Z

This is another seriously impressive chunk of work @alekstorm, thankyou! As with the h2-10 work, I'll need a little while to review the code properly. We'll also have to bikeshed the API for a while I'm afraid, because it's something I'd like to get right the first time. =)

Lukasa · 2014-04-07T06:48:34Z

hyper/http20/response.py

+
+        # The response headers. These are determined upon creation, assigned
+        # once, and never assigned again.
+        self._headers = Headers(http_headers)


You're no longer removing :status from the headers. We should do that because people don't expect to see it, and it's effectively a HTTP/2 implementation detail.

Actually, extract_from_key_value_set (above) does remove the specified keys from the key-value set - see the docstring in util.py. It's easy to miss, I know :). Any ideas for a better name?

The standard Python term for something that removes from a collection and returns would be pop, so pop_from_key_value_set is the suggested term. =) Obviously this'll need to be updated in #39.

…onse classes

…modify the set in-place

…er Response object pending further discussion

…r simplicity

alekstorm · 2014-04-07T22:06:23Z

Hokay, rebased on development (thanks for reviewing #39 so quickly!), reverted the changes to the requests adapter, and addressed most other comments.

Still TODO:

Provide an API for disabling server push (is this still a priority if we're no longer hanging pushed resources off the Response object in the requests adapter?)
Bikeshed the hyper API some more (I have some ideas here; hopefully they'll gel with yours)

…fter the original response headers

alekstorm · 2014-04-08T00:48:07Z

A closer examination of the spec made me realize that PUSH_PROMISE frames can also be sent after the response header block for the original request; this prompted a reworking of the API to support two use cases: handling pushed resources that were promised before certain checkpoints (specifically, calls to HTTP20Connection.getresponse() and HTTP20Response.read()), and handling all pushed resources as they come in, regardless of the state of the original response.

These considerations are reflected in the new HTTP20Connection.getpushes() method; see docs/source/advanced.rst for an overview. Looking forward to your feedback.

Lukasa · 2014-04-08T07:11:52Z

docs/source/advanced.rst

+closes. Using this parameter is only recommended when it is known that all
+pushed streams, or a specific one, are of higher priority than the original
+response, or when also processing the original response in a separate thread
+(N.B. do not do this; ``hyper`` is not yet thread-safe)::


I think we should reword this section of the docs slightly. What matters about getpushes() is that it returns an iterable of promises. The fact that the iterable is sometimes a list and sometimes a generator is mostly irrelevant. In fact, I think we should go further and always return a generator, even when we know we've got a list. That way we have a consistent API that prevents people doing silly things like calling len() on a generator.

Heh, reading through the code reveals that it is always a generator. In that case, just adjust the wording so that it's clearer that the return value is always a generator. Rather than the call itself blocking, it's whether the generator will block on future possible pushes. =)

Lukasa · 2014-04-08T07:31:54Z

Alright, more review comments inline. =)

As for the two TODOs:

Yeah, I still want a way to turn off Server Push. If you're not expecting them (most users won't be), we don't want to waste the bandwidth accepting them. One of my goals is to have requests users who use HTTP/2 through hyper get better performance in all (or almost all) cases than they would in HTTP/1.1. That means not letting endpoints send us extra data we're not going to use.

This API doesn't have to be clever though: it's probably enough just to have it be a flag on the HTTP20Connection object.
I like the generator API for pushed streams. My only big worry at the moment is that pushed streams have a different API to the responses we get from streams we initiated (in that they have their own object with their own methods). I want to think about whether we can do better than that.

In the meantime, if you have ideas for improving the API you should let me know!

Lukasa · 2014-04-08T07:32:17Z

Also, looks like the tests need more code coverage. =) This is small, so it should be possible to achieve without introducing #pragma no cover

…hod to HTTP20Push

alekstorm · 2014-04-08T19:33:17Z

Okey doke, thanks for the continued excellence in reviewing. I've addressed all comments (let me know if you're happy with the new wording of the getpushes() documentation), so the last TODO item is code coverage (I usually save reaching 100% for last, since the API may change under review).

Lukasa · 2014-04-08T20:50:12Z

hyper/http20/connection.py

@@ -56,6 +61,8 @@ def __init__(self, host, port=None, window_manager=None, **kwargs):
        else:
            self.host, self.port = host, port

+        self._enable_push = enable_push


This is fine for now, but it'd be an interesting enhancement to have it be a property that causes the appropriate SETTINGS frame to be emitted.

Lukasa · 2014-04-08T21:00:22Z

Alright, I pronounce this good-to-go. Let's get a green build and I'll merge the heck out of this. =D

alekstorm · 2014-04-08T23:47:53Z

Bam, done.

Lukasa · 2014-04-09T07:19:02Z

This is beautiful, I'm delighted to have this. Once again, you're just doing incredible work @alekstorm, thankyou so much. 🍰 👍

Implement server push

Lukasa · 2014-04-09T07:54:51Z

Just so you know @alekstorm, I'll be at PyCon from tomorrow through to Tuesday, which will reduce my responsiveness to pull requests and issues. Wanted to make sure you were aware. =)

alekstorm · 2014-04-10T07:43:45Z

Sure, thanks for letting me know. Have fun!

alekstorm · 2014-04-10T07:50:31Z

And thanks for the words of encouragement! They really do mean a lot :)

Lukasa · 2014-04-10T08:31:07Z

They're 100% genuine. =)

Based on your contributions over the last week or so, you've got everything you need to be a really valuable open source developer. You definitely have the technical skillset, you've demonstrated a willingness to work with maintainers to maintain style and project standards, you take code review well, and you're prepared to put in the work. You'd be a welcome contributor to any open source project you felt like.

You've done awesome work on hyper, and you should be proud of it. =)

Lukasa reviewed Apr 7, 2014
View reviewed changes

Lukasa mentioned this pull request Apr 7, 2014

Refactor Stream to ensure header blocks are decoded in the correct order #39

Merged

alekstorm added 5 commits April 7, 2014 14:41

Implement server push, exposed as the HTTP20Push and HTTP20PushedResp…

cca97fc

…onse classes

Rename extract_from_key_value_set to pop_from_key_value_set, have it …

49f254e

…modify the set in-place

Temporarily revert addition of 'pushed' attribute from requests adapt…

5b907e8

…er Response object pending further discussion

Merge BaseHTTP20Response, HTTP20Response, and HTTP20PushedResponse fo…

7f3feb4

…r simplicity

Add more comments to critical paths in Stream

d0613c8

Rework the server push API to account for PUSH_PROMISEs that arrive a…

d1ae35d

…fter the original response headers

Lukasa reviewed Apr 8, 2014
View reviewed changes

alekstorm added 5 commits April 8, 2014 12:26

Add enable_push flag to HTTP20Connection constructor and cancel() met…

f30465e

…hod to HTTP20Push

Update/clarify push docs

738b28f

Use HTTP20Connection._get_stream() in more places

cfd2773

Don't use single-item tuple deconstruction assignment for clarity

7126205

Revert remaining decorative changes to test_integration.py

d86b7e4

Fix bug: util.get_from_key_value_set() shouldn't modify its arguments

b66a6b9

Lukasa reviewed Apr 8, 2014
View reviewed changes

More testing for stream canceling and Continuation handling

ecfdb32

Lukasa added a commit that referenced this pull request Apr 9, 2014

Merge pull request #40 from alekstorm/server-push

22bb22a

Implement server push

Lukasa merged commit 22bb22a into python-hyper:development Apr 9, 2014

alekstorm deleted the server-push branch April 10, 2014 07:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement server push #40

Implement server push #40

alekstorm commented Apr 7, 2014

Lukasa Apr 7, 2014

Lukasa commented Apr 7, 2014

Lukasa Apr 7, 2014

alekstorm Apr 7, 2014

Lukasa Apr 7, 2014

alekstorm commented Apr 7, 2014

alekstorm commented Apr 8, 2014

Lukasa Apr 8, 2014

Lukasa Apr 8, 2014

Lukasa commented Apr 8, 2014

Lukasa commented Apr 8, 2014

alekstorm commented Apr 8, 2014

Lukasa Apr 8, 2014

alekstorm Apr 8, 2014

Lukasa commented Apr 8, 2014

alekstorm commented Apr 8, 2014

Lukasa commented Apr 9, 2014

Lukasa commented Apr 9, 2014

alekstorm commented Apr 10, 2014

alekstorm commented Apr 10, 2014

Lukasa commented Apr 10, 2014

Implement server push #40

Implement server push #40

Conversation

alekstorm commented Apr 7, 2014

Choose a reason for hiding this comment

Lukasa commented Apr 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alekstorm commented Apr 7, 2014

alekstorm commented Apr 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa commented Apr 8, 2014

Lukasa commented Apr 8, 2014

alekstorm commented Apr 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lukasa commented Apr 8, 2014

alekstorm commented Apr 8, 2014

Lukasa commented Apr 9, 2014

Lukasa commented Apr 9, 2014

alekstorm commented Apr 10, 2014

alekstorm commented Apr 10, 2014

Lukasa commented Apr 10, 2014