Detect unexpected response leaks for multi-address client instances #3096
Conversation
Motivation: We already apply `HttpMessageDiscardWatchdogClientFilter` to single-address clients to detect response leaks that could be caused by unhandled exceptions in the filter chain. Multi-address client adds more logic around a client group and therefore also could leak responses if case on unexpected exceptions. Modifications: - Apply `HttpMessageDiscardWatchdogClientFilter` to multi-address client instances; - Enhance `HttpMessageDiscardWatchdogClientFilter` to include unhandled exception in the logs to help narrow down the leak cause; Result: Users will see warn message if their multi-address client leaks responses.
| @@ -135,6 +135,9 @@ public StreamingHttpClient buildStreaming() { | |||
| urlClient = redirectConfig == null ? urlClient : | |||
| new RedirectingHttpRequesterFilter(redirectConfig).create(urlClient); | |||
|
|
|||
| // Detect leaks that can be caused by unexpected exceptions | |||
| urlClient = HttpMessageDiscardWatchdogClientFilter.CLIENT_CLEANER.create(urlClient); | |||
There was a problem hiding this comment.
I think this results in 2 cleaners. Maybe it's not a big deal, but just wanted to be sure I understood what the structure will be.
There was a problem hiding this comment.
Correct, because from a single address client we don't know if it's a standalone instance or part of the multi-address client group we can not remove it from there. So, for multi-address flow we will have 2 cleaners for onError case:
- Last one before returning from single-address level.
- Last one before returning from multi-address level to the users.
This should not be a big deal because the watchdog overhead mostly comes from the connection level filter that instantiates AtomicReference for each request and transforms response message body for each response. The overhead of CleanerStreamingHttpClientFilterFactory is negligible bcz onError handling is super cheap and happens only in case of exceptions.
There was a problem hiding this comment.
Sent a new commit to also replace onErrorResume with whenOnError. It will help to reduce overhead (avoids subscribe sequence for returned failed single).
There was a problem hiding this comment.
I was suggesting we could make the Single client aware if it gets instantiated from a multi client, but your explanation makes me think it's probably not a big deal. Let's be sure and check the perf stats after merge.
...tp-netty/src/main/java/io/servicetalk/http/netty/HttpMessageDiscardWatchdogClientFilter.java
Outdated
Show resolved
Hide resolved
Motivation:
We already apply
HttpMessageDiscardWatchdogClientFilterto single-address clients to detect response leaks that could be caused by unhandled exceptions in the filter chain. Multi-address client adds more logic around a client group and therefore also could leak responses if case on unexpected exceptions.Modifications:
HttpMessageDiscardWatchdogClientFilterto multi-address client instances;HttpMessageDiscardWatchdogClientFilterto include unhandled exception in the logs to help narrow down the leak cause;CleanerStreamingHttpClientFilterFactory: replaceonErrorResumewithwhenOnErrorto reduce RS overhead (avoids subscribe sequence for returned failed single);Result:
Users will see warn message if their multi-address client leaks responses.