Add ENABLE_PUSH flag in the Upgrade HTTP2-Settings header #311
Conversation
Push flag required for the case the initial upgrade request triggered server push.
Some tests requires synchronization to let the connection state machine working.
test/test_hyper.py
Outdated
| frame.data = data | ||
| if end_stream: | ||
| frame.flags.add('END_STREAM') | ||
| self.frames.append(frame) |
There was a problem hiding this comment.
Can we move these helpers out to a common mixin class now that we're using them in more than one place?
| self.http101 + b''.join([frame.serialize() | ||
| for frame in self.frames]) | ||
| ) | ||
| self.conn.request('GET', '/') |
There was a problem hiding this comment.
I wonder if this should do any asserting that the Upgrade header is present?
There was a problem hiding this comment.
Upgrading process itself is tested at test_http11.py, and IMHO, we would focus on client side view here.
test/test_integration.py
Outdated
| # Send the headers for the response. This response has no body. | ||
| f = build_headers_frame( | ||
| [(':status', '200'), ('content-length', '0')] | ||
| ) | ||
| f.flags.add('END_STREAM') | ||
| f.stream_id = 1 | ||
| sock.sendall(f.serialize()) | ||
|
|
||
| # Wait for the message from the main thread. | ||
| send_event.wait() |
There was a problem hiding this comment.
We have to wait here only when the client side are going to get the response. Another option is add send_event.wait() here, and add get_response(), send_event.set() in client side.
There was a problem hiding this comment.
So historically the reason for these waits has been to ensure that we don't close sockets in a way that leads to unexpected socket behaviour on some platforms. If we don't need to remove them I'd be inclined to leave them in.
test/test_integration.py
Outdated
| @@ -1078,8 +1126,6 @@ def socket_handler(listener): | |||
| d.data = b'1234567890' * 2 | |||
| d.flags.add('END_STREAM') | |||
| sock.send(d.serialize()) | |||
|
|
|||
| send_event.wait(5) | |||
There was a problem hiding this comment.
We have to wait here when we close the socket. Another option is add send_event.wait() and sock.close() here, and send_event.set() in client side.
There was a problem hiding this comment.
I don't think this really answered what the reason was for removing this. Was it causing a problem?
There was a problem hiding this comment.
It does not causing problem. I removed just because I thought it was not necessary.
There was a problem hiding this comment.
Yeah, for this stuff that's not really a great attitude. Specifically, these are intended to delay socket shutdown to the point where the socket is no longer in use, as some interactions can cause exceptions to be raised if EOF is received too early.
introduced FrameEncoderMixin helper class. fix waits for not displaying unwanted EOF errors.
test/test_hyper.py
Outdated
| @@ -1107,6 +1114,7 @@ def test_response_version(self): | |||
|
|
|||
|
|
|||
| class TestHTTP20Adapter(object): | |||
|
|
|||
There was a problem hiding this comment.
Why did all these blank lines get added?
There was a problem hiding this comment.
Ah... that's autopep8. I'd revert those.
test/test_integration.py
Outdated
| client_preface = b'PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n' | ||
| timeout = time.time() + 5 | ||
| got = b'' | ||
| while len(got) < len(client_preface) and time.time() < timeout: |
There was a problem hiding this comment.
What's the logic behind this timeout here? The socket doesn't actually have the timeout applied to it, so it could time out indefinitely.
There was a problem hiding this comment.
Oh, that's a bug. I used timeout for local testing.
Push flag required for the case the initial upgrade request triggered server push. This PR is a relaxed version of #310 , which was more strict on server push I/O timing.
Test send/recv timing was fixed, because connection state machine does not get corrupted during h2c upgrade migration.