[chttp2] Alternative protection for too many streams in the system#34697
[chttp2] Alternative protection for too many streams in the system#34697ctiller merged 4 commits intogrpc:masterfrom
Conversation
Automated fix for refs/heads/stream-count
| /// timeout | ||
| grpc_core::Duration keepalive_timeout; | ||
| /// number of stream objects currently allocated by this transport | ||
| std::atomic<size_t> streams_allocated{0}; |
There was a problem hiding this comment.
init_stream (which uniquely calls the constructor for grpc_chttp2_stream) is called outside the combiner;
To keep this code simple I don't want to reason about which path we're taking to get to the constructor (since it's weirdly different between client & server, etc...) - doing so would only lead to bugs (we don't want a security problem because we accidentally decremented too many times).
Other places we use similar atomics (transport level):
- every transport op
- every time a stream is created or destroyed
- every time a stream is removed
- every ping
- every read
- every write
Also at the stream level:
- every time a stream becomes writable
- every stream op (at least 2x)
This is not including any of the refcounting that happens external to the transport.
The atomic is not a concern.
There was a problem hiding this comment.
Can you point me to these atomics? I don't spot them immediately.
I still think we shouldn't be using atomics unnecessarily, but given that the transport is going through overhaul, I'm ok with this for now.
…rpc#34697) Cancel streams if we have too much concurrency on a single channel to allow that server to recover. There seems to be a convergence in the HTTP2 community about this being a reasonable thing to do, so I'd like to try it in some real scenarios. If this pans out well then I'll likely drop the `red_max_concurrent_streams` and the `rstpit` experiments in preference to this. I'm also considering tying in resource quota so that under high memory pressure we just default to this path. --------- Co-authored-by: ctiller <ctiller@users.noreply.github.com>
Cancel streams if we have too much concurrency on a single channel to allow that server to recover.
There seems to be a convergence in the HTTP2 community about this being a reasonable thing to do, so I'd like to try it in some real scenarios.
If this pans out well then I'll likely drop the
red_max_concurrent_streamsand therstpitexperiments in preference to this.I'm also considering tying in resource quota so that under high memory pressure we just default to this path.