-
Notifications
You must be signed in to change notification settings - Fork 174
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
RetryingHttpRequesterFilter use original Publisher avoid SOOE #2666
Conversation
47d61c8
to
62b4017
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.
Good idea, it should work!
final Publisher<Object> originalMessageBody = request.messageBody(); | ||
Single<StreamingHttpResponse> single = Single.defer(() -> (mayReplayRequestPayload ? | ||
delegate.request(request.transformMessageBody(messageBodyDuplicator(originalMessageBody))) : | ||
delegate.request(request.transformMessageBody(p -> originalMessageBody)) |
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.
When mayReplayRequestPayload == false
, we don't need to do anything with originalMessageBody
. Consider storing the reference and using defer
only when mayReplayRequestPayload == true
.
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.
When mayReplayRequestPayload == false, we don't need to do anything with originalMessageBody.
The unit test demonstrates the problem exists regardless of mayReplayRequestPayload
. In either case if we don't restore the original publisher we will keep appending operators and eventually SOOE.
643a54e
to
f96202c
Compare
servicetalk-concurrent-api/src/main/java/io/servicetalk/concurrent/api/RedoPublisher.java
Outdated
Show resolved
Hide resolved
f96202c
to
f343a8e
Compare
f343a8e
to
2ced001
Compare
Motivation: RetryingHttpRequesterFilter uses the request across retry operations. The request's Publisher state is shared across retries and operators applied via transformMessageBody will therefore be reapplied on each retry. This typically isn't desirable from a semantics perspective but may also lead to StackOverflowException if enough retries are attempted. Modifications: - RetryingHttpRequesterFilter should reuse the original Publisher for request payload body on each retry attempt.
2ced001
to
0b6fcdf
Compare
servicetalk-concurrent-api/src/main/java/io/servicetalk/concurrent/api/Single.java
Outdated
Show resolved
Hide resolved
servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/RetryingHttpRequesterFilter.java
Outdated
Show resolved
Hide resolved
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.
LGTM
@@ -37,7 +37,7 @@ public Single<StreamingHttpResponse> handle(final HttpServiceContext ctx, | |||
final StreamingHttpRequest request, | |||
final StreamingHttpResponseFactory responseFactory) { | |||
return request.toRequest().flatMap(req -> fromCallable(() -> original.handle( | |||
ctx, req, ctx.responseFactory())).map(HttpResponse::toStreamingResponse)); | |||
ctx, req, ctx.responseFactory())).map(HttpResponse::toStreamingResponse).shareContextOnSubscribe()); |
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.
Good catch!
Motivation:
RetryingHttpRequesterFilter uses the request across retry operations. The request's Publisher state is shared across retries and operators applied via transformMessageBody will therefore be reapplied on each retry. This typically isn't desirable from a semantics perspective but may also lead to StackOverflowException if enough retries are attempted.
Modifications: