x/net/http2: server appears to send DATA_FRAME before END_HEADERS for a stream

[bmizerany]

func TestServer_Response_LargeWrite_Concurrent(t *testing.T) {
    const size = 1 << 20
    const maxFrameSize = 16 << 10
    testServerResponse(t, func(w http.ResponseWriter, r *http.Request) error {
        n, err := w.Write(bytes.Repeat([]byte("a"), size))
        if err != nil {
            return fmt.Errorf("Write error: %v", err)
        }
        if n != size {
            return fmt.Errorf("wrong size %d from Write", n)
        }
        return nil
    }, func(st *serverTester) {
        if err := st.fr.WriteSettings(
            Setting{SettingInitialWindowSize, 0},
            Setting{SettingMaxFrameSize, maxFrameSize},
        ); err != nil {
            t.Fatal(err)
        }
        st.wantSettingsAck()

        nreqs := 1000
        streamID := uint32(1) // clients send odd ids
        for i := 0; i < nreqs; i++ {
            println("sending:", streamID)
            st.bodylessReq(streamID) // GET /
            // Give the handler quota to write:
            if err := st.fr.WriteWindowUpdate(streamID, size); err != nil {
                t.Fatal(err)
            }
            // Give the handler quota to write to connection-level
            // window as well
            if err := st.fr.WriteWindowUpdate(0, size); err != nil {
                t.Fatal(err)
            }
            streamID += 2
        }

        for {
            println("ReadFrame")
            f, err := st.fr.ReadFrame()
            if err != nil {
                panic(err)
            }
            println("nreqs:", nreqs)
            if es, ok := f.(streamEnder); ok && es.StreamEnded() {
                nreqs--
                if nreqs == 0 {
                    return
                }
            }
        }
    })
}

Running go test -run=TestServer_Response_LargeWrite_Concurrent -v hangs.

[bmizerany]

This is wrong. Ignore. Will file another.

[bmizerany]

I've updated the issue with a test hangs reproducibly.

[bmizerany]

I've submitted a CL here https://go-review.googlesource.com/#/c/15807/

[bradfitz]

The testServerResponse function is only allowed to do one write. It's even documented as such. That's where the hang is happening if you send it SIGQUIT (control-) and see the goroutines.

Have you observed this DATA-before-HEADERS behavior otherwise?

That is, is it just the test that's buggy but the issue remains?

[bradfitz]

I just audited the code for ways a DATA frame can go out before the HEADERS but I'm not seeing a way: from the handler's goroutine, (*responseWriterState).writeChunk looks fine: writeHeaders is called before writeDataFromHandler and both call writeFrameFromHandler which looks fine and sends on a channel. The sole receiver of that channel (in the server goroutine) also looks fine, calling into the write scheduler (writesched.go) and those add & push methods look fine too.

I'm closing this for now until I have something to go on.

[bmizerany]

I think this exposed what I was trying to: #12998