-
Notifications
You must be signed in to change notification settings - Fork 184
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
Avoid "Maximum active streams violated for this endpoint" at the client level #1373
Avoid "Maximum active streams violated for this endpoint" at the client level #1373
Conversation
Motivation: In apple#1307 we changed `LoadBalancedStreamingHttpClient` to not close the connection on cancel for HTTP/2 protocol, because our existing API does not expose methods for closing h2 stream instead of h2 connection. Instead, we had to _prematurely_ mark the request as finished, because in case of cancellation there is no guarantee we will receive a terminal signal on the response. Also, the stream is not aware of `LoadBalancedStreamingHttpConnection` API and therefore it can not mark the request as finished when it closes the stream. That change significantly increased a probability users see "Maximum active streams violated for this endpoint." Even though it's a `RetryableException` it creates pain and still may pop up after auto-retry strategy used all attempts. Modification: - Introduce `OwnedRunnable` that can be passed to the transport layer with the request object; - Add pkg-private `StreamingHttpRequestWithContext` wrapper which delivers `OwnedRunnable` to the transport; - Generate that `OwnedRunnable` in `LoadBalancedStreamingHttpClient`; - `LoadBalancedStreamingHttpClient` marks the request as finished only if it owns `OwnedRunnable`; - If h2-transport takes ownership of the `OwnedRunnable`, `LoadBalancedStreamingHttpClient` is not responsible for marking the request as finished anymore; - Add tests to verify the fix; Result: HTTP/2 requests marked as finished only before they reach transport or only after the stream closes.
a6c0562
to
09cf8c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few comments then lgtm for near term workaround.
...talk-http-netty/src/main/java/io/servicetalk/http/netty/H2ClientParentConnectionContext.java
Outdated
Show resolved
Hide resolved
...talk-http-netty/src/main/java/io/servicetalk/http/netty/H2ClientParentConnectionContext.java
Outdated
Show resolved
Hide resolved
return this; | ||
} | ||
|
||
static StacklessCancellationException newInstance(Class<?> clazz, String method) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need to create the new exception type, or can we just use CancellationException
and ThrowableUtils.unknownStackTrace
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we don't override fillInStackTrace()
, the Throwable
ctor will invoke it and fill in the stacktrace which ThrowableUtils.unknownStackTrace
will override later => useless expensive operation.
…nt level (#1373) Motivation: In #1307 we changed `LoadBalancedStreamingHttpClient` to not close the connection on cancel for HTTP/2 protocol, because our existing API does not expose methods for closing h2 stream instead of h2 connection. Instead, we had to _prematurely_ mark the request as finished, because in case of cancellation, there is no guarantee we will receive a terminal signal on the response. Also, the stream is not aware of `LoadBalancedStreamingHttpConnection` API and therefore it can not mark the request as finished when it closes the stream. That change significantly increased the probability users see "Maximum active streams violated for this endpoint." Even though it's a `RetryableException`, it creates pain and still may pop up after auto-retry strategy used all attempts. Modification: - Introduce `OwnedRunnable` that can be passed to the transport layer with the request object; - Add pkg-private `StreamingHttpRequestWithContext` wrapper which delivers `OwnedRunnable` to the transport; - Generate that `OwnedRunnable` in `LoadBalancedStreamingHttpClient`; - `LoadBalancedStreamingHttpClient` marks the request as finished only if it owns `OwnedRunnable`; - If h2-transport takes ownership of the `OwnedRunnable`, `LoadBalancedStreamingHttpClient` is not responsible for marking the request as finished anymore; - Add tests to verify the fix; Result: HTTP/2 requests marked as finished only before they reach transport or only after the stream closes.
Motivation:
In #1307 we changed
LoadBalancedStreamingHttpClient
to not close theconnection on cancel for HTTP/2 protocol, because our existing API does
not expose methods for closing h2 stream instead of h2 connection.
Instead, we had to prematurely mark the request as finished, because
in case of cancellation, there is no guarantee we will receive a terminal
signal on the response. Also, the stream is not aware of
LoadBalancedStreamingHttpConnection
API and therefore it can not markthe request as finished when it closes the stream.
That change significantly increased the probability users see
"Maximum active streams violated for this endpoint." Even though it's a
RetryableException
, it creates pain and still may pop up afterauto-retry strategy used all attempts.
Modification:
OwnedRunnable
that can be passed to the transport layerwith the request object;
StreamingHttpRequestWithContext
wrapper whichdelivers
OwnedRunnable
to the transport;OwnedRunnable
inLoadBalancedStreamingHttpClient
;LoadBalancedStreamingHttpClient
marks the request as finished onlyif it owns
OwnedRunnable
;OwnedRunnable
,LoadBalancedStreamingHttpClient
is not responsible for marking therequest as finished anymore;
Result:
HTTP/2 requests marked as finished only before they reach transport or
only after the stream closes.