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

H2Connection never deletes old streams. #48

Closed
Lukasa opened this issue Oct 21, 2015 · 2 comments · Fixed by #50
Closed

H2Connection never deletes old streams. #48

Lukasa opened this issue Oct 21, 2015 · 2 comments · Fixed by #50

Comments

@Lukasa
Copy link
Member

Lukasa commented Oct 21, 2015

Even when a stream is closed, H2Connection keeps the object around indefinitely. In the short term this is useful because it allows us to handle late frame arrival, but in the long term this costs memory, and also hurts performance when we scan over the streams to check which ones are open, as we will need to scan over every stream that has ever been sent.

We should come up with a policy for expiring old streams.

@Lukasa
Copy link
Member Author

Lukasa commented Oct 21, 2015

I took a quick look at nghttp2's code. It seems that nghttp2 allows up to MAX_CONCURRENT_STREAMS total state, including closed streams. We should be safely able to do that.

However, we should also make sure we don't get too aggressive about missing streams. For example, we need to tolerate receiving frames for streams that we have "forgotten": in most cases we want to send RST_STREAM if that happens, though for RST_STREAM we want to just quietly accept it, and for HEADERS we want to GOAWAY (PROTOCOL_ERROR).

This is do-able, but it's a fairly large chunk of functionality. Still worth doing though.

@Lukasa
Copy link
Member Author

Lukasa commented Oct 21, 2015

If we do this, we can also probably safely remove the closed streams altogether, rather than letting up to SETTINGS_MAX_CONCURRENT_STREAMS of them hang around. nghttp2 requires that for its priority implementation, but our priority implementation is going to be standalone and so will not require the information present in H2Connection.

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.

1 participant