Merge pull request #11719 from markdroth/large_metadata_bad_client_test_fix

bad_client test: do multiple reads if needed to read full server response

test/core/bad_client/bad_client.c

@@ -63,16 +63,9 @@ static void server_setup_transport(void *ts, grpc_transport *transport) {
   grpc_exec_ctx_finish(&exec_ctx);
 }
 
-typedef struct {
-  grpc_bad_client_client_stream_validator validator;
-  grpc_slice_buffer incoming;
-  gpr_event read_done;
-} read_args;
-
 static void read_done(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) {
-  read_args *a = arg;
-  a->validator(&a->incoming);
-  gpr_event_set(&a->read_done, (void *)1);
+  gpr_event *read_done = arg;
+  gpr_event_set(read_done, (void *)1);
 }
 
 void grpc_run_bad_client_test(
@@ -159,24 +152,31 @@ void grpc_run_bad_client_test(
   if (sfd.client != NULL) {
     // Validate client stream, if requested.
     if (client_validator != NULL) {
-      read_args args;
-      args.validator = client_validator;
-      grpc_slice_buffer_init(&args.incoming);
-      gpr_event_init(&args.read_done);
+      gpr_timespec deadline = grpc_timeout_seconds_to_deadline(5);
+      grpc_slice_buffer incoming;
+      grpc_slice_buffer_init(&incoming);
+      // We may need to do multiple reads to read the complete server response.
+      while (true) {
+        gpr_event read_done_event;
+        gpr_event_init(&read_done_event);
         grpc_closure read_done_closure;
-      GRPC_CLOSURE_INIT(&read_done_closure, read_done, &args,
+        GRPC_CLOSURE_INIT(&read_done_closure, read_done, &read_done_event,
                           grpc_schedule_on_exec_ctx);
-      grpc_endpoint_read(&exec_ctx, sfd.client, &args.incoming,
+        grpc_endpoint_read(&exec_ctx, sfd.client, &incoming,
                            &read_done_closure);
         grpc_exec_ctx_finish(&exec_ctx);
-      gpr_timespec deadline = grpc_timeout_seconds_to_deadline(5);
-      while (!gpr_event_get(&args.read_done)) {
+        do {
           GPR_ASSERT(gpr_time_cmp(deadline, gpr_now(deadline.clock_type)) > 0);
           GPR_ASSERT(grpc_completion_queue_next(
                          a.cq, grpc_timeout_milliseconds_to_deadline(100), NULL)
                          .type == GRPC_QUEUE_TIMEOUT);
+        } while (!gpr_event_get(&read_done_event));
+        if (client_validator(&incoming)) break;
+        gpr_log(GPR_INFO,
+                "client validator failed; trying additional read "
+                "in case we didn't get all the data");
       }
-      grpc_slice_buffer_destroy_internal(&exec_ctx, &args.incoming);
+      grpc_slice_buffer_destroy_internal(&exec_ctx, &incoming);
     }
     // Shutdown.
     grpc_endpoint_shutdown(

test/core/bad_client/bad_client.h

@@ -20,6 +20,9 @@
 #define GRPC_TEST_CORE_BAD_CLIENT_BAD_CLIENT_H
 
 #include <grpc/grpc.h>
+
+#include <stdbool.h>
+
 #include "test/core/util/test_config.h"
 
 #define GRPC_BAD_CLIENT_REGISTERED_METHOD "/registered/bar"
@@ -29,7 +32,8 @@ typedef void (*grpc_bad_client_server_side_validator)(grpc_server *server,
                                                       grpc_completion_queue *cq,
                                                       void *registered_method);
 
-typedef void (*grpc_bad_client_client_stream_validator)(
+// Returns false if we need to read more data.
+typedef bool (*grpc_bad_client_client_stream_validator)(
     grpc_slice_buffer *incoming);
 
 #define GRPC_BAD_CLIENT_DISCONNECT 1

test/core/bad_client/tests/large_metadata.c

@@ -30,6 +30,7 @@
 // actually appended to this in a single string, since the string would
 // be longer than the C99 string literal limit.  Instead, we dynamically
 // construct it by adding the large headers one at a time.
+
 #define PFX_TOO_MUCH_METADATA_FROM_CLIENT_PREFIX_STR                       \
   "PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n"     /* settings frame */              \
   "\x00\x00\x00\x04\x00\x00\x00\x00\x00" /* headers: generated from        \
@@ -63,15 +64,15 @@
 
 // The number of headers we're adding and the total size of the client
 // payload.
-#define NUM_HEADERS 95
+#define NUM_HEADERS 46
 #define PFX_TOO_MUCH_METADATA_FROM_CLIENT_PAYLOAD_SIZE          \
   ((sizeof(PFX_TOO_MUCH_METADATA_FROM_CLIENT_PREFIX_STR) - 1) + \
    (NUM_HEADERS * PFX_TOO_MUCH_METADATA_FROM_CLIENT_HEADER_SIZE) + 1)
 
 #define PFX_TOO_MUCH_METADATA_FROM_SERVER_STR                                              \
   "PRI * HTTP/2.0\r\n\r\nSM\r\n\r\n" /* settings frame: sets                               \
-                                        MAX_HEADER_LIST_SIZE to 16K */                     \
-  "\x00\x00\x06\x04\x00\x00\x00\x00\x00\x00\x06\x00\x00\x40\x00" /* headers:               \
+                                        MAX_HEADER_LIST_SIZE to 8K */                      \
+  "\x00\x00\x06\x04\x00\x00\x00\x00\x00\x00\x06\x00\x00\x20\x00" /* headers:               \
                                                                     generated              \
                                                                     from                   \
                                                                     simple_request.headers \
@@ -141,7 +142,7 @@ static void server_verifier_sends_too_much_metadata(grpc_server *server,
   GPR_ASSERT(0 == grpc_slice_str_cmp(call_details.host, "localhost"));
   GPR_ASSERT(0 == grpc_slice_str_cmp(call_details.method, "/foo/bar"));
 
-  const size_t metadata_value_size = 16 * 1024;
+  const size_t metadata_value_size = 8 * 1024;
   grpc_metadata meta;
   meta.key = grpc_slice_from_static_string("key");
   meta.value = grpc_slice_malloc(metadata_value_size);
@@ -166,34 +167,41 @@ static void server_verifier_sends_too_much_metadata(grpc_server *server,
   cq_verifier_destroy(cqv);
 }
 
-static void client_validator(grpc_slice_buffer *incoming) {
+static bool client_validator(grpc_slice_buffer *incoming) {
+  for (size_t i = 0; i < incoming->count; ++i) {
+    const char *s = (const char *)GRPC_SLICE_START_PTR(incoming->slices[i]);
+    char *hex = gpr_dump(s, GRPC_SLICE_LENGTH(incoming->slices[i]),
+                         GPR_DUMP_HEX | GPR_DUMP_ASCII);
+    gpr_log(GPR_INFO, "RESPONSE SLICE %" PRIdPTR ": %s", i, hex);
+    gpr_free(hex);
+  }
+
   // Get last frame from incoming slice buffer.
   grpc_slice_buffer last_frame_buffer;
   grpc_slice_buffer_init(&last_frame_buffer);
   grpc_slice_buffer_trim_end(incoming, 13, &last_frame_buffer);
   GPR_ASSERT(last_frame_buffer.count == 1);
   grpc_slice last_frame = last_frame_buffer.slices[0];
+
   const uint8_t *p = GRPC_SLICE_START_PTR(last_frame);
-  // Length = 4
-  GPR_ASSERT(*p++ == 0);
-  GPR_ASSERT(*p++ == 0);
-  GPR_ASSERT(*p++ == 4);
+  bool success =
+      // Length == 4
+      *p++ != 0 || *p++ != 0 || *p++ != 4 ||
       // Frame type (RST_STREAM)
-  GPR_ASSERT(*p++ == 3);
+      *p++ != 3 ||
       // Flags
-  GPR_ASSERT(*p++ == 0);
+      *p++ != 0 ||
       // Stream ID.
-  GPR_ASSERT(*p++ == 0);
-  GPR_ASSERT(*p++ == 0);
-  GPR_ASSERT(*p++ == 0);
-  GPR_ASSERT(*p++ == 1);
+      *p++ != 0 || *p++ != 0 || *p++ != 0 || *p++ != 1 ||
       // Payload (error code)
-  GPR_ASSERT(*p++ == 0);
-  GPR_ASSERT(*p++ == 0);
-  GPR_ASSERT(*p++ == 0);
-  GPR_ASSERT(*p == 0 || *p == 11);
+      *p++ == 0 || *p++ == 0 || *p++ == 0 || *p == 0 || *p == 11;
+
+  if (!success) {
+    gpr_log(GPR_INFO, "client expected RST_STREAM frame, not found");
+  }
 
   grpc_slice_buffer_destroy(&last_frame_buffer);
+  return success;
 }
 
 int main(int argc, char **argv) {