HTTP/2 test server by makdharma · Pull Request #8900 · grpc/grpc

makdharma · 2016-11-29T20:29:36Z

HTTP2 protocol test cases.

makdharma · 2016-12-09T23:35:14Z

test this please

ncteisen

Looked over all file and made some comments. Approving because nothing depends on our new tests yet.

In the future I still think it makes sense to change the server design to never close, and somehow determine what test to run per request (maybe by using the metadata of the req). That way the client never needs to ensure the server is running the test case of the same type. That seems cleaned than a scrip to start them both up

ncteisen · 2016-12-12T15:56:55Z

test/http2_test/http2_base_server.py

+    if self._conn.data_to_send:
+      self.transport.write(self._conn.data_to_send())
+    for event in events:
+      if isinstance(event, RequestReceived) and self._handlers.has_key('RequestReceived'):


For this section maybe we should change the logic to be if is instance of EventType but _handlers does not have the key, throw an exception, or print some error.

Because if we receive and event that we haven't set up a handler for, wouldn't it be better to fail than to do nothing?

This is not an error case. There will be events that we don't need to process, example RemoteSettingsChanged.

ncteisen · 2016-12-12T16:10:27Z

test/http2_test/http2_base_server.py

+        break
+      self._send_remaining[stream_id] -= bytes_to_send
+      self._send_offset += bytes_to_send
+      if self._send_remaining[stream_id] == 0:


This could be pulled out of the while loop

Not really. default_send gets called every time window_update is received. pulling it out of while loop will cause it to send trailer even when no data was sent.

ncteisen · 2016-12-12T16:12:19Z

test/http2_test/http2_base_server.py

+
+  def parse_received_data(self, stream_id):
+    recv_buffer = self._recv_buffer[stream_id]
+    """ returns a grpc framed string of bytes containing response proto of the size


Put docstring line directly after function definition

ncteisen · 2016-12-12T16:14:08Z

test/http2_test/http2_base_server.py

+      #logging.error('not enough data to decode req proto. size = %d, needed %s'%(len(recv_buffer), 5+grpc_msg_size))
+      return None
+    req_proto_str = recv_buffer[5:5+grpc_msg_size]
+    sr = messages_pb2.SimpleRequest()


Looks like all this can parse for now is a SimpleRequest. Probably makes sense in the future to accepts all supported TestService types

ncteisen · 2016-12-12T16:18:09Z

test/http2_test/test_goaway.py

+
+  def on_send_done(self, stream_id):
+    self._base_server.on_send_done_default(stream_id)
+    time.sleep(1)


Is this needed to avoid race conditions? Maybe add a comment rather than have the naked sleep

Not really needed.. leftover from hack. removed.

ncteisen · 2016-12-12T16:20:00Z

test/http2_test/test_max_streams.py

+class TestcaseSettingsMaxStreams(object):
+  """
+    This test sets MAX_CONCURRENT_STREAMS to 1 and asserts that at any point
+    only 1 stream is active.


Mentioned that all the assertions happen at the twisted layer. Otherwise its confusing when people look through the file and don't see any assertions.

We should also specify somewhere that the client should first make a single RPC and block on the response before initiating this test. The C++ client can/will otherwise send 10 concurrent streams and complete before it receives the MAX_CONCURRENT_STREAMS setting from the server.

I added a TODO for now.

ncteisen · 2016-12-12T16:21:21Z

test/http2_test/http2_base_server.py

+      if self._send_remaining[stream_id] == 0:
+        self._handlers['SendDone'](stream_id)
+
+  def default_ping(self):


Add logging? It would make the ping test clearer

ncteisen · 2016-12-12T16:21:32Z

test/http2_test/http2_base_server.py

+        self._handlers['PingAcknowledged'](event)
+    self.transport.write(self._conn.data_to_send())
+
+  def on_ping_acknowledged_default(self, event):


Add logging?

ericgribkoff

Mostly minor style comments, plus a few questions about clarifying test specifications.

ericgribkoff · 2016-12-12T17:09:53Z

test/http2_test/http2_base_server.py

@@ -0,0 +1,173 @@
+import struct
+import messages_pb2
+import logging


Ordering: should be in lexicographical order.

ericgribkoff · 2016-12-12T17:11:27Z

test/http2_test/http2_base_server.py

+from twisted.internet import reactor
+from h2.connection import H2Connection
+from h2.events import RequestReceived, DataReceived, WindowUpdated, RemoteSettingsChanged, PingAcknowledged
+from h2.exceptions import ProtocolError


Ordering: h2 should come before twisted. The Python style guide also recommends avoiding importing individual classes instead of modules.

ericgribkoff · 2016-12-12T17:11:30Z

test/http2_test/http2_base_server.py

+from h2.exceptions import ProtocolError
+
+READ_CHUNK_SIZE = 16384
+GRPC_HEADER_SIZE = 5


These look like internal constants, and should start with an underscore, _READ_CHUNK_SIZE, _GRPC_HEADER_SIZE.

ericgribkoff · 2016-12-12T17:15:29Z

test/http2_test/http2_base_server.py

+    self.transport.write(self._conn.data_to_send())
+
+  def on_connection_lost(self, reason):
+    logging.info('Disconnected %s'%reason)


space before and after the % sign: logging.info('Disconnected %s' % reason)

I think this should also be done in the other format strings below

ericgribkoff · 2016-12-12T17:21:25Z

test/http2_test/http2_base_server.py

+  def dataReceived(self, data):
+    try:
+      events = self._conn.receive_data(data)
+    except ProtocolError:


Can this catch a more specific exception, like StreamClosedError from https://github.com/python-hyper/hyper-h2/blob/master/h2/exceptions.py?

I could only make it generate ProtocolError in my testing.

ericgribkoff · 2016-12-12T17:37:29Z

test/http2_test/http2_test_server.py

+
+class H2Factory(Factory):
+  def __init__(self, testcase):
+    logging.info('In H2Factory')


Changed log message. This is useful for debugging when a new connection is made.

ericgribkoff · 2016-12-12T17:42:30Z

test/http2_test/test_rst_during_data.py

+      response_len = len(response_data)
+      truncated_response_data = response_data[0:response_len/2]
+      self._base_server.setup_send(truncated_response_data, event.stream_id)
+      # send reset stream


Remove or move comment? on_send_done() sends the reset stream, so not clear what this means here.

ericgribkoff · 2016-12-12T17:48:13Z

test/http2_test/test_rst_after_data.py

+  """
+    In response to an incoming request, this test sends headers, followed by
+    data, followed by a reset stream frame. Client asserts that the RPC failed.
+    Client needs to deliver the complete message to the application layer.


Are we sure that all client languages can do this? For instance,Java can only deliver the completed message to the application layer using the async API. I think PHP, at least, might still be missing async calls.

Let's propose changes after we make inventory of what is possible for each language.

ericgribkoff · 2016-12-12T17:51:05Z

test/http2_test/test_max_streams.py

+class TestcaseSettingsMaxStreams(object):
+  """
+    This test sets MAX_CONCURRENT_STREAMS to 1 and asserts that at any point
+    only 1 stream is active.


We should also specify somewhere that the client should first make a single RPC and block on the response before initiating this test. The C++ client can/will otherwise send 10 concurrent streams and complete before it receives the MAX_CONCURRENT_STREAMS setting from the server.

ericgribkoff · 2016-12-12T17:57:38Z

test/http2_test/test_goaway.py

+    This test does the following:
+      Process incoming request normally, i.e. send headers, data and trailers.
+      Then send a GOAWAY frame with the stream id of the processed request.
+      It assert that the next request is made on a different TCP connection.


s/assert/asserts

More importantly, where is the assertion done? If this is relying on H2/Twisted internals to catch any errors, maybe add that to comments?

This is actually not an assertion but the comment on line 27 explains the logic.

ericgribkoff · 2016-12-12T22:12:19Z

test/http2_test/test_goaway.py

+  def on_connection_lost(self, reason):
+    logging.info('Disconnect received. Count %d' % self._iteration)
+    # _iteration == 2 => Two different connections have been used.
+    if self._iteration == 2:


After looking at the base server implementation, I think I understand what this accomplishes: if _iteration != 2, the server won't acknowledge the connection lost, and consequently the server won't be shutdown (which happens from _base_server.on_connection_lost()). This would then be regarded as a failed test.

Would it be better to "fail fast" here and throw an exception if _iteration != 2? Otherwise, it seems like the test runner would have to wait until the test timeouts to see that the server is still running and the test failed, if I understand how the test driver will execute.

ericgribkoff · 2016-12-13T18:28:09Z

test/http2_test/http2_test_server.py

+
+if __name__ == "__main__":
+  logging.basicConfig(format = "%(levelname) -10s %(asctime)s %(module)s:%(lineno)s | %(message)s", level=logging.INFO)
+  parser = argparse.ArgumentParser()


I think this would be easier to use with a help message that lists the test cases. The following snippet adds help messages, and switches to --flag=value style arguments to be consistent with the existing interop test binaries. I also assigned a default value to --port - this makes things easier for me when running locally for testing. Feel free to just take the help messages if you prefer positional arguments.

if __name__ == '__main__': logging.basicConfig( format='%(levelname) -10s %(asctime)s %(module)s:%(lineno)s | %(message)s', level=logging.INFO) parser = argparse.ArgumentParser() parser.add_argument('--test', choices=_TEST_CASE_MAPPING.keys(), help='test case to run') parser.add_argument('--port', type=int, default=25678, help='port to run the server (default: 25678)') args = parser.parse_args() if args.test not in _TEST_CASE_MAPPING.keys(): logging.error('unknown test: %s' % args.test) else: endpoint = twisted.internet.endpoints.TCP4ServerEndpoint( twisted.internet.reactor, args.port, backlog=128) endpoint.listen(H2Factory(args.test)) twisted.internet.reactor.run()

I also added some line wrapping. I think we probably want to stay <= 80 columns, which I didn't check for when I was reviewing this yesterday.

Maybe sorting the _TEST_CASE_MAPPING keys is better to keep a consistent ordering in the help message:

parser.add_argument('--test', choices=sorted(_TEST_CASE_MAPPING.keys()), help='test case to run')

makdharma · 2016-12-15T16:52:13Z

Merging since python issues are unrelated to this PR and the code in this PR is not currently used anywhere.

y-zeng · 2016-12-15T22:29:00Z

Looks like check_copyright.py is not happy.
https://grpc-testing.appspot.com/job/gRPC_master_linux/798/console

ncteisen · 2016-12-15T22:38:43Z

Fixing PR is #9131

dgquintas · 2016-12-15T23:07:55Z

This PR has cause sanity to start failing: https://grpc-testing.appspot.com/job/gRPC_master_linux/lastCompletedBuild/testReport/(root)/sanity_linux_dbg/tools_distrib_check_copyright_py/

Also, please check the tests before merging and follow the guidelines regarding mentioning the failures in the PR description. eg: #9089 (comment)

These tests are for the new HTTP/2 test server (grpc/grpc#8900). They are designed to test client behavior when the server sends unexpected responses during rpcs.

experimental

af37b80

googlebot added the cla: yes label Nov 29, 2016

makdharma added 6 commits December 1, 2016 10:51

Broke up code in multiple modules..

323bfa7

minor cleanup..

28d1980

ability to deal with multiple streams in flight.

4350e74

WIP

ecc3288

bugfix in rst_after_data

5b7070a

added new test (rst_during_data)

a16ea7f

makdharma changed the title ~~experimental~~ HTTP/2 test server Dec 5, 2016

ncteisen requested review from ericgribkoff and ncteisen December 9, 2016 23:46

ncteisen approved these changes Dec 12, 2016

View reviewed changes

ericgribkoff reviewed Dec 12, 2016

View reviewed changes

fixed feedback from review

8c57917

ericgribkoff reviewed Dec 12, 2016

View reviewed changes

ericgribkoff approved these changes Dec 12, 2016

View reviewed changes

ericgribkoff reviewed Dec 13, 2016

View reviewed changes

ericgribkoff mentioned this pull request Dec 13, 2016

interop-testing: New client for HTTP/2 "negative" interop tests grpc/grpc-java#2513

Merged

This was referenced Dec 13, 2016

Add Python Client to Test Negative HTTP2 Conditions #9097

Merged

Add Go Client to Test Negative HTTP2 Conditions grpc/grpc-go#1022

Merged

makdharma merged commit 2249e92 into grpc:master Dec 15, 2016

ericgribkoff mentioned this pull request Dec 16, 2016

Add HTTP/2 interop test client #9139

Merged

makdharma deleted the http2_test branch June 9, 2017 23:39

lock bot locked as resolved and limited conversation to collaborators Jan 23, 2019

Conversation

makdharma commented Nov 29, 2016

Uh oh!

makdharma commented Dec 9, 2016

Uh oh!

ncteisen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericgribkoff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment