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

FINAL and WINDOW_UPDATE #104

Closed
martinthomson opened this issue May 29, 2013 · 8 comments
Closed

FINAL and WINDOW_UPDATE #104

martinthomson opened this issue May 29, 2013 · 8 comments

Comments

@martinthomson
Copy link
Collaborator

FINAL is currently defined as a promise not to send any more frames on the stream.

This doesn't work with flow control.

A GET request will result in the client -> server stream direction being half-closed after sending just the headers. However, the client is required to send WINDOW_UPDATE frames to enable the receipt of packets from the server.

The same problem applies to PRIORITY. Re-prioritization might be desirable for a stream that you have half-closed. (For instance, it might be good to down-prioritize a GET after completing the request.)

The obvious workaround just creates a stalemate. A client can't just retain the stream in the open state until it sees the FINAL flag from a server. That encourages the same solution on the server, which leads to neither peer closing the connection.

It seems like the most pragmatic solution to this is to permit the sending of certain types of frames on half-closed streams.

@grmocg
Copy link
Contributor

grmocg commented Jun 7, 2013

Flow control should only apply to DATA frames, so is this an issue?

@martinthomson
Copy link
Collaborator Author

Yes. Imagine that you send a GET request for a large file. You have an initial window size of 64K. You send HEADERS+PRIORITY with a FINAL bit set. The server sends you 64K of data, but then you can't send a WINDOW_UPDATE because you promised that you wouldn't send any more frames on that stream. Deadlock.

@grmocg
Copy link
Contributor

grmocg commented Jun 7, 2013

Got it.

For me, the distinction is 'on' vs 'about', which we really haven't defined well, and absolutely should.

PUSH_PROMISE, HEADERS, HEADERS+PRIORITY and DATA are all stream content (without them, we'd not be able to give the endpoint the data necessary to do HTTP processing on the request/response), whereas the rest of the frames are simply talking about those streams.

@martinthomson
Copy link
Collaborator Author

That might be a useful distinction to make. The obvious question is then whether we make that distinction generically accessible or not. It's almost like that would create a new data/control distinction again (data, not DATA).

@martinthomson
Copy link
Collaborator Author

Oh, and I see your related email about ordering. I think that if we were to make such a distinction, then ordering - not just the FINAL bit - would apply to the set of "data" frames that you identified. Actually "data" is wrong, maybe "protocol semantic bearing" would be more correct, if a little stuffy and pompous-sounding.

@jasnell
Copy link
Contributor

jasnell commented Jun 7, 2013

This gets back to the "Tiers" discussion I raised a while back. Flow control is part of the first tier while stream processing is second tier. I know that terminology sucks but the idea ought to be clear enough. We need to clearly define the tiers/layers then define the processing requirements around those... specifically, sending a RST_STREAM means that no more second tier frames are sent, but things that operate on the first tier are perfectly fine.

@grmocg
Copy link
Contributor

grmocg commented Jun 7, 2013

I think we can bound it better than that, but yea.
I think we're all agreeing and seeking the appropriate terminology.
Once we've found that (or before), we definitely need to take it to the list.

@mnot
Copy link
Member

mnot commented Jun 13, 2013

Discussed at SF Interim; waiting for proposal from Layering TF.

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

No branches or pull requests

4 participants