-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Fix connection leak in rest proxy when return type is void or mono<void> #30072
Conversation
API change check API changes are not detected in this pull request. |
return service.setBinaryData(endpoint, id, binaryDataSupplier.get(), length) | ||
.then(); | ||
return Mono.fromSupplier(binaryDataSupplier) | ||
.flatMap(data -> service.setBinaryData(endpoint, id, data, length)); |
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.
Just curious, why is the new code better?
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.
The binaryDataSupplier
is supposed to supply fresh BinaryData
for each runAsync
.
The old code is effectively doing that, but only because the caller uses flatMap
- which is just luck rather than proper reactive stream.
Wrapping the supplier in Mono makes it properly reactive, i.e. subscriber triggers binary data creation regardless of what caller of this is doing.
@@ -206,7 +206,7 @@ private Object handleRestReturnType(final Mono<HttpResponseDecoder.HttpDecodedRe | |||
final Type monoTypeParam = TypeUtil.getTypeArgument(returnType); | |||
if (TypeUtil.isTypeOrSubTypeOf(monoTypeParam, Void.class)) { | |||
// ProxyMethod ReturnType: Mono<Void> | |||
result = asyncExpectedResponse.then(); | |||
result = asyncExpectedResponse.doOnNext(HttpResponseDecoder.HttpDecodedResponse::close).then(); |
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.
Should this be doOnNext
or doOnSuccess
? Does Mono<Void>
emit a next or does it only emit completion?
This applies to the void scenario below as well.
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.
asyncExpectedResponse is Mono<Mono<HttpResponseDecoder.HttpDecodedResponse>
when we add that operator.
doOnSuccess
would be kind of same here, but we'd need to null check in the consumer.
see
https://projectreactor.io/docs/core/release/api/reactor/core/publisher/Mono.html#doOnNext-java.util.function.Consumer-
|
Caught in perf pipelines.
Rest Proxy would leak connections if interface method returns void or Mono like this
azure-sdk-for-java/sdk/core/azure-core-perf/src/main/java/com/azure/core/perf/core/MyRestProxyService.java
Lines 39 to 43 in cf1b7dc
The symptoms were perf of OkHttp client grossly underperforming in core perf pipeline. Attempt to reproduce revealed OkHttp complaining about undisposed responses.