Respect maxConcurrentStreams #2327
Conversation
997175d to
0732ade
Compare
Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
0732ade to
c628091
Compare
There was a problem hiding this comment.
I went back and forth on this a bunch but I think this is the right thing.
At first I was going to suggest that we should really only count streams in the StreamOpen state and not StreamLocalReset or StreamFailed. Unfortunately, this is more complicated because we definitely don't want to have to iterate over the whole stream map each time we need to decide if a new stream can be started.
But given that the whole point of this is to protect us from memory leaks caused by badly behaved clients, perhaps it does make sense to count everything in the streams map. We may want to add a comment explaining why we count everything in the streams map and not just StreamOpen (otherwise, future code spelunkers may see this and wonder if it's a bug/oversight)
|
@adleong We can easily avoid iterration by maintaining counters or splitting things in multiple maps. I think that adds complexity to begin with. Also I think counting streams that are in StreamLocalReset is the correct thing to do. After all these streams are still occupying some sort of resources. I think it is correct to assume that in well behaved scenarios these streams will be evicted from the map once the other side closes as well, so its safe to simply do a On another note, had to google what a spelunkers is...
Spot on! Probably the best word to describe anyone who delves into the Netty4BaseDispatcher code |
Signed-off-by: Zahari Dichev <zaharidichev@gmail.com>
This commit adds the functionality to respect maxConcurrentStreams for server dispatchers. The reason for this is that H2 settings such as
maxConcurrentStreamsare merely advertisement based. We as a server need to protect against misbehaving clients that do not respect the settings we have advertised. Otherwise we are prone to memory leaks and all sorts of resource depletion problems.This PR is a limited version of #2325 which was a more opinionated version of this work.
Signed-off-by: Zahari Dichev zaharidichev@gmail.com