h2spec: GOAWAY is needed for receiving frames after a stream closed.

[zizhong]

@masaori335 @maskit With #1704, #1747, #1752, and this fix, all test cases of h2spec passed!

Hypertext Transfer Protocol Version 2 (HTTP/2)
  3. Starting HTTP/2
    3.5. HTTP/2 Connection Preface
      ✔ 1: Sends client connection preface
      ✔ 2: Sends invalid connection preface
...
...
...
    8.2. Server Push
      ✔ 1: Sends a PUSH_PROMISE frame

Finished in 1.3408 seconds
94 tests, 94 passed, 0 skipped, 0 fail

[masaori335]

[approve ci]

[atsci]

RAT check successful! https://ci.trafficserver.apache.org/job/RAT-github/358/

[atsci]

clang format successful! https://ci.trafficserver.apache.org/job/clang-format-github/344/

[masaori335]

@zizhong Nice work!

[atsci]

Linux build successful! https://ci.trafficserver.apache.org/job/linux-github/1932/

[atsci]

AU check successful! https://ci.trafficserver.apache.org/job/autest-github/342/

[atsci]

FreeBSD11 build successful! https://ci.trafficserver.apache.org/job/freebsd-github/2040/

[atsci]

Intel CC build successful! https://ci.trafficserver.apache.org/job/icc-github/471/

[atsci]

clang-analyzer build successful! https://ci.trafficserver.apache.org/job/clang-analyzer-github/604/

[maskit]

I'm not sure whether we should change it, it may make h2spec happy though.

An endpoint that receives any frame other than PRIORITY
after receiving a RST_STREAM MUST treat that as a stream error
(Section 5.4.2) of type STREAM_CLOSED. Similarly, an endpoint
that receives any frames after receiving a frame with the
END_STREAM flag set MUST treat that as a connection error
(Section 5.4.1) of type STREAM_CLOSED, unless the frame is
permitted as described below.

Are we sure that this is the second case here?

[zizhong]

This commit is to fix the issue as follows

Hypertext Transfer Protocol Version 2 (HTTP/2)
  5. Streams and Multiplexing
    5.1. Stream States
           [send] SETTINGS Frame (length:6, flags:0x00, stream_id:0)
           [recv] SETTINGS Frame (length:12, flags:0x00, stream_id:0)
           [send] SETTINGS Frame (length:0, flags:0x01, stream_id:0)
           [recv] WINDOW_UPDATE Frame (length:4, flags:0x00, stream_id:0)
           [recv] SETTINGS Frame (length:0, flags:0x01, stream_id:0)
           [send] HEADERS Frame (length:12, flags:0x05, stream_id:1)
           [recv] HEADERS Frame (length:206, flags:0x05, stream_id:1)
           [send] DATA Frame (length:4, flags:0x01, stream_id:1)
           [recv] DATA Frame (length:0, flags:0x01, stream_id:1)
           [recv] RST_STREAM Frame (length:4, flags:0x00, stream_id:1)
           [recv] Timeout
      × 11: closed: Sends a DATA frame
        -> The endpoint MUST treat this as a connection error of type STREAM_CLOSED.
           Expected: GOAWAY Frame (Error Code: STREAM_CLOSED)
                     Connection closed
             Actual: RST_STREAM Frame (length:4, flags:0x00, stream_id:1)

Here it is definitely the second case.

[maskit]

Well, the h2spec testcase checks the second case, but what I wanted to know is whether the line is used only for the second case. If not, we might be breaking something else which is not covered by h2spec.

Give me a few more days to check it.

[zizhong]

Yes. No guarantee that this code path is only for the second case. It's for every call on a destroyed stream. I feel like ATS is too aggressive on closing streams.

[maskit]

I confirmed that the line is used for the both cases.

I tested it with h2seq and these data.

[                                                                                   
    { "action": "send", "stream": 0, "type": "preface"},                            
    { "action": "send", "stream": 0, "type": "settings", "flags": "",    "settings": [] },
    { "action": "send", "stream": 0, "type": "settings", "flags": "ACK", "settings": [] },
    { "action": "send", "stream": 1, "type": "headers",  "flags": "END_HEADERS,END_STREAM", "headers": [                                                                                                                                                      
        [":method", "GET"],                                                         
        [":scheme", "https"],                                                       
        [":authority", "localhost"],                                                
        [":path", "/"]                                                              
    ]},                                                                             
    { "action": "pause", "duration": 3000 },                                        
    { "action": "send", "stream": 1, "type": "data"},                               
    { "action": "pause", "duration": 3000 }                                         
]

[                                                                                   
    { "action": "send", "stream": 0, "type": "preface"},                            
    { "action": "send", "stream": 0, "type": "settings", "flags": "",    "settings": [] },
    { "action": "send", "stream": 0, "type": "settings", "flags": "ACK", "settings": [] },
    { "action": "send", "stream": 1, "type": "headers",  "flags": "END_HEADERS", "headers": [                                                                                                                                                                 
        [":method", "GET"],                                                         
        [":scheme", "https"],                                                       
        [":authority", "localhost"],                                                
        [":path", "/"]                                                              
    ]},                                                                             
    { "action": "send", "stream": 1, "type": "rst_stream"},                         
    { "action": "pause", "duration": 3000 },                                        
    { "action": "send", "stream": 1, "type": "data"},                               
    { "action": "pause", "duration": 3000 }                                         
]

[maskit]

I agree that ATS need to be fixed, however, I'd have to request changes because it fixes one case but also breaks another case.

Maybe we could use some existing flags or have to add a flag to know which case has occurred.

[maskit]

It seems each streams have their end state (recv_end_stream) but we can't use it here, since Http2Stream instances are not available. So, I think we have to manage closed streams with their end state. The closed streams need to be managed separately from stream_list in Http2ConnectionState because we need to keep stream_list as short as possible.

Also, according to the spec, the check need to be done not only for DATA frames but also for other frames.

@zizhong Would you try this?

[zizhong]

@maskit Good suggestion. Let me see.

[calavera]

is there any change on this @zizhong? it would be nice to have a fully compatible release of ATS with H2.

[zizhong]

@calavera Yes, I will work on this one after finishing some other stuff.

[bryancall]

@zizhong What is the status on this and should we land this?

[zizhong]

@bryancall This needs some future work. Can you put a WIP tag for me? Thanks!

[bryancall]

There hasn't been an update on this PR for over 60 days, so I am closing it for now. Feel free to reopen the PR when you have updated it.