Adding session windows to the flow control section #4

Merged
merged 2 commits into from Feb 6, 2013

Conversation

Projects
None yet
2 participants

willchan commented Feb 6, 2013

Reading through the flow control section, it needs a lot of work. But here's a start.

+ information upstream back to the original sender.) Flow control only
+ applies to the data portion of data frames. Recipients must buffer
+ all control frames. If a recipient fails to buffer an entire control
+ frame, it MUST issue a stream error (Section 2.4.2) with the status
@grmocg

grmocg Feb 6, 2013

Owner

This seems wrong. I suspect what we want here is to cease to read upon the connection, or close it down.
A stream error could be fatal to the session here for the wrong frame types, e.g. any which contain a Header Block and so is unsafe.

@willchan

willchan Feb 6, 2013

This isn't new.

@willchan

willchan Feb 6, 2013

And I agree with your comment. Note that our existing SPDY/3 specs and implementation are broken in this manner.

+ After sending each data frame, the sender decrements both the per
+ stream window size and the session window size by the amount of data
+ transmitted. When a stream window size becomes less than or equal to
+ 0, the sender must pause transmitting data frames for that stream.
@grmocg

grmocg Feb 6, 2013

Owner

must pause is a bit ambiguous.

How about:
When a stream window size is less than or equal to
0, the sender MUST NOT send a data frame, unless it is a zero-length data frame with the FIN (or whatever it is called) flag set.

As an aside, we could allow zero-length data frames without a FIN to indicate that the stream is blocked (a max of one). We shouldn't put that in now.

@willchan

willchan Feb 6, 2013

This section isn't new either. I agree with your comments and thought so myself went adding the session window. I would like to do that separately from cleaning up existing problems with the spec, as I mean to propose this to the HTTPbis WG as per the action item given me. I think we should do the editorial cleanups in a separate change.

+ transmitted. When a stream window size becomes less than or equal to
+ 0, the sender must pause transmitting data frames for that stream.
+ Likewise, when the session window size becomes less than or equal to
+ 0, the sender must pause transmitting any data frames for the
@grmocg

grmocg Feb 6, 2013

Owner

this should be similarly changed

@willchan

willchan Feb 6, 2013

I agree, but like the other suggestions, I'd like to postpone them to a separate change to really clean up this section.

is always 4.
Stream-ID: The stream ID that this WINDOW_UPDATE control frame is
- for.
+ for. If the stream ID value is 0, the WINDOW_UPDATE frame applies to
@grmocg

grmocg Feb 6, 2013

Owner

I don't think we've spec'd what happens when a receiver receives a WINDOW_UPDATE frame for one of its incoming streams. I'd guess we should say it should be ignored (it is another possible venue for indicating one is blocked)...

@willchan

willchan Feb 6, 2013

I thought about the WINDOW_UPDATE for the wrong direction of stream id as well. I'm inclined to treat it as a session error for now.

+ limit, then if the Stream-ID was 0, it MUST send a GOAWAY frame with
+ status code FLOW_CONTROL_ERROR to terminate the session. And if the
+ Stream-ID references an active stream, it must send a RST_STREAM
+ frame with status code FLOW_CONTROL_ERROR to terminate the stream.
@grmocg

grmocg Feb 6, 2013

Owner

Perhaps we should add something like: This is better than simply capping at 2^31 because it makes it significantly less likely for a buggy implementation to stall forever.

@willchan

willchan Feb 6, 2013

That's a good explanation of why to do it this way. Do specs often indicate why they say MUST or SHOULD or whatever? I'm happy to put this in if desired.

+ 16KB, and the sender sends 64KB for a stream immediately on session
+ establishment, the sender will discover its window size is -48KB on
+ receipt of the SETTINGS. As the recipient consumes the first 16KB,
+ it can send a WINDOW_UPDATE of 16KB back to the sender. This
@grmocg

grmocg Feb 6, 2013

Owner

We probably need a setting for session and for stream separately. Bleh.,

@willchan

willchan Feb 6, 2013

I had considered that, but decided it was unnecessary. The reason is that you should just send a WINDOW_UPDATE with stream id 0 with whatever delta to increase the session window size accordingly. In order to shrink it, you just let the sender send data and don't send WINDOW_UPDATE for the session window until it goes below the desired size.

The reason to prefer a SETTINGS frame is just for persistence. I don't think we're considering persisting it as of yet.

I did consider whether or not to rename the SETTINGS id to indicate it's only for the stream window. I can do that if desired.

Owner

grmocg commented Feb 6, 2013

I've added in some in-line notes.

willchan commented Feb 6, 2013

I've responded to all the inline notes I saw. I agree with all the comments and thought of them myself while writing this down. I think the wording needs a fair amount of cleanup. I'm not sure if we should do that before or after submitting the wording to the working group. In any case, I'd like to do the cleanup/refactor as a separate pull request so this one is strictly about session windows.

grmocg added a commit that referenced this pull request Feb 6, 2013

Merge pull request #4 from willchan/grmocg-gh-pages
Adding session windows to the flow control section

@grmocg grmocg merged commit 41abb89 into grmocg:gh-pages Feb 6, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment