Repository: grpc/grpc-go
File: internal/transport/http2_client.go (Go)

Code before:
	if t.state == closing {
		t.mu.Unlock()
		return
	}
	if t.logger.V(logLevel) {
		t.logger.Infof("Closing: %v", err)
	}
	// Call t.onClose ASAP to prevent the client from attempting to create new
	// streams.
	if t.state != draining {
		t.onClose(GoAwayInvalid)
	}
	t.state = closing
	streams := t.activeStreams
	t.activeStreams = nil
	if t.kpDormant {
		// If the keepalive goroutine is blocked on this condition variable, we
		// should unblock it so that the goroutine eventually exits.
		t.kpDormancyCond.Signal()
	}
	t.mu.Unlock()

	// Append info about previous goaways if there were any, since this may be important
	// for understanding the root cause for this connection to be closed.
	_, goAwayDebugMessage := t.GetGoAwayReason()

	var st *status.Status
	if len(goAwayDebugMessage) > 0 {
		st = status.Newf(codes.Unavailable, "closing transport due to: %v, received prior goaway: %v", err, goAwayDebugMessage)
		err = st.Err()
	} else {
		st = status.New(codes.Unavailable, err.Error())
	}

	// Per HTTP/2 spec, a GOAWAY frame must be sent before closing the
	// connection. See https://httpwg.org/specs/rfc7540.html#GOAWAY. It
	// also waits for loopyWriter to be closed with a timer to avoid the
	// long blocking in case the connection is blackholed, i.e. TCP is
	// just stuck.
	t.controlBuf.put(&goAway{code: http2.ErrCodeNo, debugData: []byte("client transport shutdown"), closeConn: err})
	timer := time.NewTimer(goAwayLoopyWriterTimeout)
	defer timer.Stop()
	select {
	case <-t.writerDone: // success
	case <-timer.C:
		t.logger.Infof("Failed to write a GOAWAY frame as part of connection close after %s. Giving up and closing the transport.", goAwayLoopyWriterTimeout)
	}
	t.cancel()
	t.conn.Close()
	channelz.RemoveEntry(t.channelz.ID)


Review comment (bug):
The current implementation of `http2Client.outgoingGoAwayHandler` holds `t.mu` when calling `WriteGoAway` on the underlying framer, i.e. it is performing I/O when holding the lock. Is this the correct thing to do?

Can/should this be replaced with the following:
```golang
// OutgoingGoAwayHandler writes a GOAWAY to the connection.  Always returns
// (false, err) as we want the GoAway to be the last frame loopy writes to the
// transport.
func (t *http2Client) outgoingGoAwayHandler(g *goAway) (bool, error) {
	t.mu.Lock()
	nextID := t.nextID - 2
	t.mu.Unlock()
	if err := t.framer.fr.WriteGoAway(nextID, http2.ErrCodeNo, g.debugData); err != nil {
		return false, err
	}
	return false, g.closeConn
}
```



Code after:
	// Make sure we only close once.
	if t.state == closing {
		t.mu.Unlock()
		return
	}
	if t.logger.V(logLevel) {
		t.logger.Infof("Closing: %v", err)
	}
	// Call t.onClose ASAP to prevent the client from attempting to create new
	// streams.
	if t.state != draining {
		t.onClose(GoAwayInvalid)
	}
	t.state = closing
	streams := t.activeStreams
	t.activeStreams = nil
	if t.kpDormant {
		// If the keepalive goroutine is blocked on this condition variable, we
		// should unblock it so that the goroutine eventually exits.
		t.kpDormancyCond.Signal()
	}
	// Append info about previous goaways if there were any, since this may be important
	// for understanding the root cause for this connection to be closed.
	goAwayDebugMessage := t.goAwayDebugMessage
	t.mu.Unlock()

	// Per HTTP/2 spec, a GOAWAY frame must be sent before closing the
	// connection. See https://httpwg.org/specs/rfc7540.html#GOAWAY. It
	// also waits for loopyWriter to be closed with a timer to avoid the
	// long blocking in case the connection is blackholed, i.e. TCP is
	// just stuck.
	t.controlBuf.put(&goAway{code: http2.ErrCodeNo, debugData: []byte("client transport shutdown"), closeConn: err})
	timer := time.NewTimer(goAwayLoopyWriterTimeout)
	defer timer.Stop()
	select {
	case <-t.writerDone: // success
	case <-timer.C:
		t.logger.Infof("Failed to write a GOAWAY frame as part of connection close after %s. Giving up and closing the transport.", goAwayLoopyWriterTimeout)
	}
	t.cancel()
	t.conn.Close()
	channelz.RemoveEntry(t.channelz.ID)
	var st *status.Status
	if len(goAwayDebugMessage) > 0 {
		st = status.Newf(codes.Unavailable, "closing transport due to: %v, received prior goaway: %v", err, goAwayDebugMessage)
		err = st.Err()
	} else {
		st = status.New(codes.Unavailable, err.Error())
	}

	// Notify all active streams.