Remove streams closed long ago from closed stream structure #242
Conversation
…ure' of github.com:awslabs/aws-c-http into remove-stream-closed-long-ago-from-closed-stream-structure
TingDaoK
changed the title
| #define AWS_H2_MIN_WINDOW_SIZE (256) | ||
| /* We will ignore frames for closed stream within this time slot, after that we will treat them as connection error: | ||
| * nano secs, 100000000 nano secs -> 0.1 sec */ | ||
| #define AWS_H2_IGNORE_TIME (100000000) |
There was a problem hiding this comment.
I don't really know how long it should be, and the spec seems let us make our own choice.
There was a problem hiding this comment.
We should see if we can get some inspiration from other H2 implementations, check node's or chrome's maybe?
There was a problem hiding this comment.
naming/comment suggestion "ignore time" is just too vague. Also, the frames aren't always ignored, they can cause stream errors too
/* Once a stream is closed, the connection remembers details about it for this long.
* After that, the details are cleaned up and any frames that arrive for the stream
* are treated as a connection error. */
AWS_H2_CLOSED_STREAM_MEMORY_TIME
There was a problem hiding this comment.
So, I looked into Netty and Nghttp2 to see how they deal with the close stream. And they both not exactly follow the spec to define a time slot for ignoring the frame...🤦 And Chrome is just too complicated...
There was a problem hiding this comment.
Used LRU cache instead of a fixed time.
There was a problem hiding this comment.
Oh, one possible case? Someone open a lot of streams and keep sending RST_STREAM on them, the LRU cache will keep those closed streams as MRU, and the other recently closed stream will be kept out of the cache, and we will result in treating all the others as connection error.🤦And we will keep sending RST_STREAM back...
There was a problem hiding this comment.
Finially, FIFO cache instead of LRU cache!
justinboswell
left a comment
justinboswell
left a comment
There was a problem hiding this comment.
Functionally, looks pretty good, just a few naming/clarity things
| #define AWS_H2_MIN_WINDOW_SIZE (256) | ||
| /* We will ignore frames for closed stream within this time slot, after that we will treat them as connection error: | ||
| * nano secs, 100000000 nano secs -> 0.1 sec */ | ||
| #define AWS_H2_IGNORE_TIME (100000000) |
There was a problem hiding this comment.
We should see if we can get some inspiration from other H2 implementations, check node's or chrome's maybe?
| #define AWS_H2_MIN_WINDOW_SIZE (256) | ||
| /* We will ignore frames for closed stream within this time slot, after that we will treat them as connection error: | ||
| * nano secs, 100000000 nano secs -> 0.1 sec */ | ||
| #define AWS_H2_IGNORE_TIME (100000000) |
There was a problem hiding this comment.
naming/comment suggestion "ignore time" is just too vague. Also, the frames aren't always ignored, they can cause stream errors too
/* Once a stream is closed, the connection remembers details about it for this long.
* After that, the details are cleaned up and any frames that arrive for the stream
* are treated as a connection error. */
AWS_H2_CLOSED_STREAM_MEMORY_TIME
…ure' of github.com:awslabs/aws-c-http into remove-stream-closed-long-ago-from-closed-stream-structure
…ure' of github.com:awslabs/aws-c-http into remove-stream-closed-long-ago-from-closed-stream-structure
| /* When window size is too small to fit the possible padding into it, we stop sending data and wait for WINDOW_UPDATE */ | ||
| #define AWS_H2_MIN_WINDOW_SIZE (256) | ||
| /* Default value for thread_data.max_closed_stream_cache_size */ | ||
| #define AWS_H2_DEFAULT_MAX_CACHE_SIZE (32) |
There was a problem hiding this comment.
naming suggestion: AWS_H2_DEFAULT_MAX_CLOSED_STREAMS
"cache" is pretty vague unless you know the code
also, the comment refers to a variable that doesn't exist anymore
source/h2_connection.c
Outdated
| * MUST treat that as a stream error (Section 5.4.2) of type STREAM_CLOSED */ | ||
|
|
||
| /* Stream was closed long ago, or didn't fit criteria for being ignored */ | ||
| /* Stream was closed long ago (or implicitly closed when its ID was skipped) */ |
There was a problem hiding this comment.
trivial: "long ago" doesn't really apply anymore
/* Stream closed (purged from closed_streams, or implicitly closed when its ID was skipped) */
TingDaoK
deleted the
remove-stream-closed-long-ago-from-closed-stream-structure
branch
- Used FIFO cache to store the closed streams.
- Remove streams closed from the closed stream cache, once the cache is full.
- Fixed different type of error we report at closed state of stream. Details are as followed:
1. Receive RST_STREAM, then receive frames soon after: stream error of type STREAM_CLOSED
2. Receive END_STREAM, then receive WINDOW_UPDATE and RST_STREAM soon after: ignore frames
3. Receive END_STREAM, then receive frames other than WINDOW_UPDATE and RST_STREAM soon after: connection error of type STREAM_CLOSED
4. Send RST_STREAM, then receive frames soon after: ignore frames
5. Stream closed loooong ago, then receive any frames: connection error of type
PROTOCOL_ERROR
Co-authored-by: Dengke Tang <dengket@amazon.com>
4 participants
Issue #, if available:
PROTOCOL_ERROR
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.