Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

initial flow control window size state clarification #727

Closed
Scottmitch opened this issue Mar 17, 2015 · 16 comments · Fixed by #736
Closed

initial flow control window size state clarification #727

Scottmitch opened this issue Mar 17, 2015 · 16 comments · Fixed by #736

Comments

@Scottmitch
Copy link

section 6.9.2 has the following text:

A SETTINGS frame can alter the initial flow control window size for
all streams in the "open" or "half closed (remote)" state. When the
value of SETTINGS_INITIAL_WINDOW_SIZE changes, a receiver MUST adjust
the size of all stream flow control windows that it maintains by the
difference between the new value and the old value.

Draft 17 added the changed current streams to streams in the "open" or "half closed (remote)" state. Can someone clarify why would streams in other states be excluded?

@martinthomson
Copy link
Collaborator

Those are the only states in which data (or DATA) can be sent.

@Scottmitch
Copy link
Author

Does your stream have to be eligible for data to be sent to have its windows manipulated? For example are IDLE streams intentionally excluded? Would this mean that the initial settings frame wouldn't apply to any streams? I think it may make sense to be able to alter window sizes of streams in other states because of dependencies/priorities and how the windows are distributed amongst the tree.

@martinthomson
Copy link
Collaborator

While it is true than an idle stream is affected, the text there is to highlight the fact that streams with active flow control windows (i.e., ones that might be in use) might be affected.

@Scottmitch
Copy link
Author

Is it possible to clarify this? I think there are a few options:

  1. If we are going to explicitly call out states: we should call out all states where it could possibly impact streams.
  2. If we are not going to explicitly call out states: Use a description which implies stream state which is defined elsewhere in the spec. Something like: all future streams and any stream subject to flow control or in the flow control dependency tree. Note this is just an example...

The current text in the specification is in conflict with this clarification you have provided in this issue. It seems like the following sentence in the spec may be sufficient by itself?

@Scottmitch
Copy link
Author

@martinthomson - Ping.

@martinthomson
Copy link
Collaborator

In addition to changing the flow control window for future streams, a SETTINGS frame can alter the initial flow control window size for streams with active flow control windows (that is, streams in the "open" or "half closed (remote)" state).

@Scottmitch
Copy link
Author

The inclusion of stream state in your clerical response leads me to another question related to the priority tree, stream state, and flow control windows. Section 5.3.2 indicates that Streams with the same parent SHOULD be allocated resources proportionally based on their weight. The flow control window is a resource which can be divided amongst nodes in the priority tree. However the PRIORITY frame can be sent/received in all states, but the WINDOW_UPDATE frame is more restrictive in its allowed states. This discrepancy makes controlling the resource (the window) difficult (i.e. providing a mechanism to avoid deadlock). Is there any way to clarify or allow the resource (the window) to be controlled adequately along with priority?

@martinthomson
Copy link
Collaborator

As far as it goes, yes, a server might allocate flow control window based on priority. But that only applies to the logic that an implementation uses to decide how to allocate credit to flow control windows, which is intentionally (and expressly) unspecified. I don't think that it needs a special call-out.

@Scottmitch
Copy link
Author

I agree that it doesn't need to be specifically called out that the window can be treated as a resource, and that this is an implantation detail. However calling out the stream states when referencing frames that impact the flow control window may be overly restrictive. For example the "open" or "half closed (remote)" are not the only states in which a stream may be in the priority tree.

Here are what I believe to be the relevant portions of the specification which relate to flow control window and stream state.

  1. section 6.9 seems to be very careful about stream state and doesn't necessarily limit to just "open" or "half closed (remote)"
  2. section 5.1 is much more restrictive and declares that WINDOW_UPDATE is not allowed in idle, or closed. For closed there is special mention that these frames must be ignored, or may be an error (if it arrives a significant time after sending END_STREAM).
  3. If the clarifications in this issue are applied then it seems like the SETTINGS impact on the flow control windows is further limited to "open" or "half closed (remote)".

@martinthomson
Copy link
Collaborator

BTW, I consider the state associated with having a stream in the priority tree to be necessarily separate from the state that is maintained for an "active stream" (i.e., one with active send or receive state). Lumping them all together exposes you to some potential attacks.

@Scottmitch
Copy link
Author

Can you be more specific (particularly about the potential attacks)? I guess what I'm after is two fold:

  1. Consistency amongst the sections that discuss window update and stream state.
  2. Not necessarily preventing window_update being controlled when a stream is in the priority tree, or rational as to why it makes sense to do so.

@martinthomson
Copy link
Collaborator

The amount of state you commit to a stream that is active is potentially large (flow control windows, primarily). That is strictly bounded though by your value of the MAX_CONCURRENT_STREAMS setting. If that state has to live on the same lifetime as stream priority, then it is not covered by the setting and you are potentially in a position to commit a much larger amount of state.

You can (obviously) manage this by careful management of the transition to and from the active states, but I believe that it is better to instead have separate tracking of priority.

@martinthomson
Copy link
Collaborator

One way to implement this cleanly is to have a (small) priority state for streams that is created and managed by a garbage-collector. Active stream state can be a separate object that maintains a reference to the priority state (that might be nullable, or you could require that only closed or idle streams be exposed to the GC).

@Scottmitch
Copy link
Author

I think we are straying into implementation details a bit, and we are in agreement that the state could be managed, but in the scope of this issue....I re-read your PR and I am fine with it :)

@laike9m
Copy link

laike9m commented Nov 24, 2016

@martinthomson Does "old value" here means the old initial window size?

@martinthomson
Copy link
Collaborator

Yes

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

Successfully merging a pull request may close this issue.

3 participants