HTTP/1 respects the stream window. #277
Conversation
Also break up giant process_read_message() function into smaller chunks. Previously, we were only queueing messages after switching protocols and becoming a midchannel handler. Now we always queue messages before processing them. This is in preparation for changing how windows work in HTTP/1. We might need to buffer up read messages if the HTTP-stream's window is too small.
| #include <aws/http/status_code.h> | ||
| #include <aws/io/logging.h> | ||
|
|
||
| #include <inttypes.h> |
There was a problem hiding this comment.
What made this necessary, it should be covered by common.h?
There was a problem hiding this comment.
I used PRIu64 in a LOGF, which comes from inttypes.h
you're thinking of stuff like uint64_t which comes from stdint.h, which we do cover in common.h
source/h1_connection.c
Outdated
|
|
||
| const size_t increment_size = aws_sub_size_saturating(desired_size, connection->thread_data.window_size); | ||
| if (increment_size > 0) { | ||
| connection->thread_data.window_size += increment_size; |
There was a problem hiding this comment.
Should we not record this until AFTER we successfully update the window on the channel?
There was a problem hiding this comment.
My logic here works by calculating the desired window, and issuing a request for the difference between desired and current.
The window_size I'm updating here is NOT the official window according to the channel (that would be aws_channel_slot.window_size). That variable isn't updated until the channel runs the window update task. If I based my math off that variable, I could end up asking for too much because there are outstanding updates that haven't landed yet.
I'll add comments explaining this...
There was a problem hiding this comment.
I had the thought that we might want to modify how aws_channel_slot.window_size works, so every channel handler doesn't feel the need to duplicate it for its own internal math.
Like, keep using a task to inform a handler's neighbor that its window has been updated, but keep aws_channel_slot.window_size as up to date as possible
| if ((pending_window_update > 0) && (api_state == AWS_H1_STREAM_API_STATE_ACTIVE)) { | ||
| /* Now that stream window is larger, connection might have buffered | ||
| * data to send, or might need to increment its own window */ | ||
| aws_h1_connection_try_process_read_messages(connection); |
There was a problem hiding this comment.
It might be better to early stop here if the stream is not the current incoming stream?
There was a problem hiding this comment.
eh, that's complicated. the function can handle it
doesn't seem worth optimizing for since it's pretty rare for a connection to have multiple streams
- more thoughtful use of assert/saturate/runtime-error - rename the two `window_size` variables to `connection_window` and `stream_window` for clarity.
bretambrose
left a comment
bretambrose
left a comment
There was a problem hiding this comment.
Couple things to consider
Co-authored-by: Dengke Tang <dengket@amazon.com>
- capacity is allowed to be smaller than initial_read_window - 0 means "pick something for me", this is the default
4 participants
ISSUE
Previously, the "stream window" in HTTP/1 was a lie. There was really just the connection-window. We encountered bugs when a connection was re-used for another stream. The stream believed it started with initial-window-size, but really it got whatever connection-window was left over from the previous stream. The connection could stall because the connection-window size was nearly zero, but the stream thought its window was wide open. Likewise, if the previous stream left a larger connection-window, the new stream could receive data that exceeded its initial-window-size.
SOLUTION
Honor the stream window. All new streams start with initial-window-size. Streams cannot receive more body data than their stream-window allows. Users can no longer control the connection window directly, because it is doing gymnastics to honor these contracts.
DETAILS
aws_io_messages are put in a queue before processing. This allows the connection to receive more data than a stream's window might allow, and process the data later when the stream's window opens, or the next stream begins. The connection-window is capped at
read_buffer_capacity, and shrinks when there is unprocessed data in the queue.users can customize
read_buffer_capacity, but we'll choose something for them if they leave it 0 in the options.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.