Merge branch 'proxy-crash' into server_stall

src/core/transport/chttp2/internal.h

@@ -65,6 +65,7 @@ typedef enum {
   GRPC_CHTTP2_LIST_WRITTEN,
   GRPC_CHTTP2_LIST_PARSING_SEEN,
   GRPC_CHTTP2_LIST_CLOSED_WAITING_FOR_PARSING,
+  GRPC_CHTTP2_LIST_CLOSED_WAITING_FOR_WRITING,
   GRPC_CHTTP2_LIST_STALLED_BY_TRANSPORT,
   /** streams that are waiting to start because there are too many concurrent
       streams on the connection */
@@ -284,6 +285,9 @@ struct grpc_chttp2_transport_parsing {
   gpr_slice goaway_text;
 
   gpr_int64 outgoing_window;
+
+  /** pings awaiting responses */
+  grpc_chttp2_outstanding_ping pings;
 };
 
 struct grpc_chttp2_transport {
@@ -392,8 +396,6 @@ typedef struct {
   gpr_uint8 write_closed;
   /** is this stream reading half-closed (boolean) */
   gpr_uint8 read_closed;
-  /** is this stream finished closing (and reportably closed) */
-  gpr_uint8 finished_close;
   /** is this stream in the stream map? (boolean) */
   gpr_uint8 in_stream_map;
   /** has this stream seen an error? if 1, then pending incoming frames
@@ -587,6 +589,13 @@ int grpc_chttp2_list_pop_closed_waiting_for_parsing(
     grpc_chttp2_transport_global *transport_global,
     grpc_chttp2_stream_global **stream_global);
 
+void grpc_chttp2_list_add_closed_waiting_for_writing(
+    grpc_chttp2_transport_global *transport_global,
+    grpc_chttp2_stream_global *stream_global);
+int grpc_chttp2_list_pop_closed_waiting_for_writing(
+    grpc_chttp2_transport_global *transport_global,
+    grpc_chttp2_stream_global **stream_global);
+
 grpc_chttp2_stream_parsing *grpc_chttp2_parsing_lookup_stream(
     grpc_chttp2_transport_parsing *transport_parsing, gpr_uint32 id);
 grpc_chttp2_stream_parsing *grpc_chttp2_parsing_accept_stream(

src/core/transport/chttp2/stream_lists.c

@@ -353,6 +353,26 @@ int grpc_chttp2_list_pop_closed_waiting_for_parsing(
   return r;
 }
 
+void grpc_chttp2_list_add_closed_waiting_for_writing(
+    grpc_chttp2_transport_global *transport_global,
+    grpc_chttp2_stream_global *stream_global) {
+  stream_list_add(TRANSPORT_FROM_GLOBAL(transport_global),
+                  STREAM_FROM_GLOBAL(stream_global),
+                  GRPC_CHTTP2_LIST_CLOSED_WAITING_FOR_WRITING);
+}
+
+int grpc_chttp2_list_pop_closed_waiting_for_writing(
+    grpc_chttp2_transport_global *transport_global,
+    grpc_chttp2_stream_global **stream_global) {
+  grpc_chttp2_stream *stream;
+  int r = stream_list_pop(TRANSPORT_FROM_GLOBAL(transport_global), &stream,
+                          GRPC_CHTTP2_LIST_CLOSED_WAITING_FOR_WRITING);
+  if (r != 0) {
+    *stream_global = &stream->global;
+  }
+  return r;
+}
+
 void grpc_chttp2_register_stream(grpc_chttp2_transport *t,
                                  grpc_chttp2_stream *s) {
   stream_list_add_tail(t, s, GRPC_CHTTP2_LIST_ALL_STREAMS);

src/core/transport/chttp2_transport.c

@@ -134,6 +134,9 @@ static void connectivity_state_set(
 static void check_read_ops(grpc_exec_ctx *exec_ctx,
                            grpc_chttp2_transport_global *transport_global);
 
+static void fail_pending_writes(grpc_exec_ctx *exec_ctx, 
+                                grpc_chttp2_stream_global *stream_global);
+
 /*
  * CONSTRUCTION/DESTRUCTION/REFCOUNTING
  */
@@ -625,6 +628,7 @@ void grpc_chttp2_terminate_writing(grpc_exec_ctx *exec_ctx,
                                    void *transport_writing_ptr, int success) {
   grpc_chttp2_transport_writing *transport_writing = transport_writing_ptr;
   grpc_chttp2_transport *t = TRANSPORT_FROM_WRITING(transport_writing);
+  grpc_chttp2_stream_global *stream_global;
 
   GPR_TIMER_BEGIN("grpc_chttp2_terminate_writing", 0);
 
@@ -638,6 +642,11 @@ void grpc_chttp2_terminate_writing(grpc_exec_ctx *exec_ctx,
 
   grpc_chttp2_cleanup_writing(exec_ctx, &t->global, &t->writing);
 
+  while (grpc_chttp2_list_pop_closed_waiting_for_writing(&t->global, &stream_global)) {
+    fail_pending_writes(exec_ctx, stream_global);
+    GRPC_CHTTP2_STREAM_UNREF(exec_ctx, stream_global, "finish_writes");
+  }
+
   /* leave the writing flag up on shutdown to prevent further writes in unlock()
      from starting */
   t->writing_active = 0;
@@ -1113,6 +1122,16 @@ void grpc_chttp2_fake_status(grpc_exec_ctx *exec_ctx,
   }
 }
 
+static void fail_pending_writes(grpc_exec_ctx *exec_ctx, 
+                                grpc_chttp2_stream_global *stream_global) {
+  grpc_chttp2_complete_closure_step(
+      exec_ctx, &stream_global->send_initial_metadata_finished, 0);
+  grpc_chttp2_complete_closure_step(
+      exec_ctx, &stream_global->send_trailing_metadata_finished, 0);
+  grpc_chttp2_complete_closure_step(exec_ctx,
+                                    &stream_global->send_message_finished, 0);
+}
+
 void grpc_chttp2_mark_stream_closed(
     grpc_exec_ctx *exec_ctx, grpc_chttp2_transport_global *transport_global,
     grpc_chttp2_stream_global *stream_global, int close_reads,
@@ -1129,12 +1148,13 @@ void grpc_chttp2_mark_stream_closed(
   }
   if (close_writes && !stream_global->write_closed) {
     stream_global->write_closed = 1;
-    grpc_chttp2_complete_closure_step(
+    if (TRANSPORT_FROM_GLOBAL(transport_global)->writing_active) {
-        exec_ctx, &stream_global->send_initial_metadata_finished, 0);
+      GRPC_CHTTP2_STREAM_REF(stream_global, "finish_writes");
-    grpc_chttp2_complete_closure_step(
+      grpc_chttp2_list_add_closed_waiting_for_writing(transport_global,
-        exec_ctx, &stream_global->send_trailing_metadata_finished, 0);
+                                                      stream_global);
-    grpc_chttp2_complete_closure_step(exec_ctx,
+    } else {
-                                      &stream_global->send_message_finished, 0);
+      fail_pending_writes(exec_ctx, stream_global);
+    }
   }
   if (stream_global->read_closed && stream_global->write_closed) {
     if (stream_global->id != 0 &&
@@ -1146,7 +1166,6 @@ void grpc_chttp2_mark_stream_closed(
         remove_stream(exec_ctx, TRANSPORT_FROM_GLOBAL(transport_global),
                       stream_global->id);
       }
-      stream_global->finished_close = 1;
       GRPC_CHTTP2_STREAM_UNREF(exec_ctx, stream_global, "chttp2");
     }
   }
@@ -1360,7 +1379,6 @@ static void recv_data(grpc_exec_ctx *exec_ctx, void *tp, int success) {
       GPR_ASSERT(stream_global->write_closed);
       GPR_ASSERT(stream_global->read_closed);
       remove_stream(exec_ctx, t, stream_global->id);
-      stream_global->finished_close = 1;
       GRPC_CHTTP2_STREAM_UNREF(exec_ctx, stream_global, "chttp2");
     }
   }

tools/http2_interop/s6.5.go

@@ -1,6 +1,7 @@
 package http2interop
 
 import (
+	"fmt"
 	"time"
 )
 
@@ -30,3 +31,60 @@ func testSmallMaxFrameSize(ctx *HTTP2InteropCtx) error {
 
 	return nil
 }
+
+// Section 6.5.3 says all settings frames must be acked.
+func testAllSettingsFramesAcked(ctx *HTTP2InteropCtx) error {
+	conn, err := connect(ctx)
+	if err != nil {
+		return err
+	}
+	defer conn.Close()
+	conn.SetDeadline(time.Now().Add(defaultTimeout))
+
+	sf := &SettingsFrame{}
+	if err := http2Connect(conn, sf); err != nil {
+		return err
+	}
+
+	// The spec says "The values in the SETTINGS frame MUST be processed in the order they
+	// appear. [...] Once all values have been processed, the recipient MUST immediately
+	// emit a SETTINGS frame with the ACK flag set."  From my understanding, processing all
+	// of no values warrants an ack per frame.
+	for i := 0; i < 10; i++ {
+		if err := streamFrame(conn, sf); err != nil {
+			return err
+		}
+	}
+
+	var settingsFramesReceived = 0
+	// The server by default sends a settings frame as part of the handshake, and another
+	// after the receipt of the initial settings frame as part of our conneection preface.
+	// This means we expected 1 + 1 + 10 = 12 settings frames in return, with all but the
+	// first having the ack bit.
+	for settingsFramesReceived < 12 {
+		f, err := parseFrame(conn)
+		if err != nil {
+			return err
+		}
+
+		// Other frames come down the wire too, including window update.  Just ignore those.
+		if f, ok := f.(*SettingsFrame); ok {
+			settingsFramesReceived += 1
+			if settingsFramesReceived == 1 {
+				if f.Header.Flags&SETTINGS_FLAG_ACK > 0 {
+					return fmt.Errorf("settings frame should not have used ack: %v")
+				}
+				continue
+			}
+
+			if f.Header.Flags&SETTINGS_FLAG_ACK == 0 {
+				return fmt.Errorf("settings frame should have used ack: %v", f)
+			}
+			if len(f.Params) != 0 {
+				return fmt.Errorf("settings ack cannot have params: %v", f)
+			}
+		}
+	}
+
+	return nil
+}

tools/http2_interop/s6.5_test.go

@@ -13,3 +13,14 @@ func TestSoonSmallMaxFrameSize(t *testing.T) {
 	err := testSmallMaxFrameSize(ctx)
 	matchError(t, err, "Got goaway frame")
 }
+
+func TestSoonAllSettingsFramesAcked(t *testing.T) {
+	defer Report(t)
+	if *testCase != "framing" {
+		t.SkipNow()
+	}
+	ctx := InteropCtx(t)
+	if err := testAllSettingsFramesAcked(ctx); err != nil {
+		t.Fatal(err)
+	}
+}

tools/http2_interop/settings.go

@@ -26,6 +26,10 @@ const (
 	SettingsMaxHeaderListSize    SettingsIdentifier = 6
 )
 
+const (
+	SETTINGS_FLAG_ACK byte = 0x01
+)
+
 func (si SettingsIdentifier) String() string {
 	switch si {
 	case SettingsHeaderTableSize: